0

I am running into this issue where I want the largest and smallest value of id from a file that I am reading. This file contains other information as well. I am successfully able to identify the largest value but the smallest is just being set to the last value that gets read, which is not the smallest in the file. Here is the code

largestId = 0;
smallestId = 99999;
while(theFile >> firstName >> lastName >> id)
{
   if(id > largestId){
            largestId = id;
   }else if(id < smallestId){
            smallestId = id;
   }
}
k4b00m
  • 105
  • 6
  • 5
    Why are you using an else if? The two conditions should not rely on each other. – NathanOliver Sep 15 '21 at 16:16
  • @NathanOliver When I use If the whole thing breaks and I get the original values of 0 and 99999 as the largest and smallest. – k4b00m Sep 15 '21 at 16:19
  • Are you saying having `if(id > largestId){ largestId = id;} if(id < smallestId){ smallestId = id; }` doesn't work? – NathanOliver Sep 15 '21 at 16:20
  • Never mind it's working now. I have no clue how, since i did the exact same thing before and it was not but thanks @NathanOliver – k4b00m Sep 15 '21 at 16:22
  • 1
    Think about the id 1 and the first iteration. It is both greater than `largestId` and less than `smallestId`, but you only set the former. That you get the last number is an artefact of your particular input. – molbdnilo Sep 15 '21 at 16:23
  • 1
    `largestId = std::max(largestId, id);` is clearer than `if`. – Evg Sep 15 '21 at 16:26
  • Note: For FP types, concerns arise with `-0.0, NAN`. See [Z boson](https://stackoverflow.com/a/30915238/2410359) – chux - Reinstate Monica Sep 15 '21 at 17:32

2 Answers2

4

You don't check for error states. What about the situation were there are no values in the file. Or only one value in the file.

You make assumptions about the size of largest and smallest values by using magic numbers.

if (theFile >> firstName >> lastName >> id)
{
    // You have at least one value it is the largest and smallest value.
    smallestId = largestId = id;


    while(theFile >> firstName >> lastName >> id)
    {
       // There is an argument that this would be better to write
       // as a function call as that would be self documenting.
       //  id = std::max(id, largestId);
       if(id > largestId){
                largestId = id;
       }
       if(id < smallestId){
                smallestId = id;
       }
    }
}
else {
    // ERROR
}
Martin York
  • 257,169
  • 86
  • 333
  • 562
  • In this case with `smallestId = largestId = id;`, later code could use `if(id < smallestId)` --> `else if(id < smallestId)`: a [micro optimization](https://softwareengineering.stackexchange.com/questions/80084/is-premature-optimization-really-the-root-of-all-evil). – chux - Reinstate Monica Sep 15 '21 at 17:27
1

Here is more fancy way you could do it by using algorithms and iterators:

struct Person 
{
    std::string firstName;
    std::string lastName;
    int id;
};

std::istream& operator>>(std::istream& in, Person& person)
{
    return in >> person.firstName >> person.lastName >> person.id;
}

bool find_min_max(std::istream& in, int& min, int& max)
{
    using Iter = std::istream_iterator<Person>;
    auto a = std::minmax_element(Iter{in}, {},
        [](const auto& a, const auto& b) { return a.id < b.id; });
        
    if (a.first != Iter{}) 
    {
        min = a.first->id;
        max = a.second->id;
    }

    return a.first != Iter{};
}

https://godbolt.org/z/dY8KqzzxM

Marek R
  • 32,568
  • 6
  • 55
  • 140