Share via

SOLUTION : Spotting Code Defects #3 (C++ 101)

  • Article

As always thanks to those that emailed responses. Please feel free to post them in the feedback area too!

The biggest problem with this piece of code is that the stream is not being checked for errors after reading. Look at this block:

    int age;

    cin >> age;

    people[name] = age;

What happens when a invalid value (anything non-integer, for example) is given as input? The stream read fails and cin has some flags set to indicate that failure. What you should do is something more like the following (though this is not how I would actually ever write it – but without changing the over-all program flow…)

So first prompt. Next attempt to read. If the stream is still good then set our success flag and get out. But if not then first clear the error bits. Do this so anything else can happen. Next ignore the first character in the stream. Perhaps they entered “a32” – that is invalid so ignore the “a”. Then peek into the stream. Are we are eof()? If so then abort. Perhaps stdin came from a file or pipe and that is EOF or closed. Who knows. Basically we keep trying until it succeeds. Removing one character at a time from the failure stream until either it succeeds or until the >> operator has nothing to work with and blocks on stdin for input again.

bool readAge = false;

int age;

cout << "Adding a new person! Enter " << name << "\'s age: ";

while(true)

{

    cin >> age;

    if(cin)

    {

    readAge = true;

    break;

    }

    else

  {

    cin.clear();

    cin.ignore();

    if(cin.peek() == char_traits<char>::eof())

    {

    cout << "End of stream detected. Aborting" << endl;

    break;

    }

    }

}

if(readAge)

{

    people[name] = age;

    cout << "Added " << name << endl;

}

else

{

    break;

}

Is this the right thing to do?

 

That depends on your goals. This example takes “a32”, skips the “a” and uses the “32”. But what if they entered “a32bbb”? Well, the “a” is skipped, the 32 is used and the next cin still contains “bbb\n” (notice the newline is still there) – so when we loop back to prompt for a name there is one already on stdin. That’s probably not what we want.

 

The easiest way to handle this is to simply clear the line after reading an age … something like this:

 

cin >> age;

if(cin)

{

      cin.ignore(numeric_limits<int>::max(), '\n');

      readAge = true;

      break;

}

This really doesn’t excite me that much but it works well enough for now.

 

Of course we use cin’s extraction operator (>>) twice so we’d need to do this twice. And now the function is huge. So large I’m not going to paste it here.

 

It’s not long before we start looking at other solutions. Perhaps we need a custom extractor that does all of this for us. A very straight forward one would be something like this:

 

template<class T>

istream& readFromStream(istream &in, T &obj, bool discardRestOfLine = true)

{

      while(true)

      {

            in >> obj;

            if(in)

            {

                  if(discardRestOfLine)

                  {

                        in.ignore(numeric_limits<int>::max(), '\n');

                  }

                  break;

            }

            else

            {

                  in.clear();

                  in.ignore();

                  if(in.peek() == char_traits<char>::eof())

                  {

                        cout << "End of stream detected. Aborting" << endl;

                        break;

                  }

            }

      }

      return in;

}

 

 

Then we could replace our original function to look like this:

 

 

bool readAge = false;

int age;

cout << "Adding a new person! Enter " << name << "\'s age: ";

if(readFromStream(cin, age))

{

      readAge = true;

}

if(readAge)

{

      people[name] = age;

      cout << "Added " << name << endl;

}

else

{

      break;

}

 

I’m still not really happy with this though. There are just so many edge conditions where things could go wrong. But I think it’s enough for C++ 101.

 

There are still a lot of problems I have with this code. The logic is wrong (negative ages?), there is a performance problem here:

 

if(people.find(name) != people.end())

{

      cout << name << " is " << people[name] << " years old." << endl;

}

We do the look up twice. Something like this would be better:

 

map<string,int>::iterator lookUp = people.find(name);

if(lookUp != people.end())

{

      cout << name << " is " << lookUp->second << " years old." << endl;

}

 

Why pay the penalty twice?

 

Bottom line is that the code has problems. Aside from the stream bug there are several serious design problems we won’t even attempt to address J If this were something beyond C++ 101 we’d be talking about refactoring. But it’s not. So in the spirit of the original I didn’t want to change too much.

 

Thoughts?

Comments