5

I'm having a problem with what should be incredibly simple code. I want to take in an integer between 1 and 3 with error checking. It works fine for checking for numbers that are too large or too small, but when a alpha/number combination is entered, it gets stuck in an infinite loop. Suggestions?

#include <iostream>
using namespace std;

int main(int argc, char *argv[]){
    int input;

    cout << "\nPlease enter a number from 1 to 3:" << endl;
    cout << "-> ";
    cin >> input;

    while(input< 1 || input> 3){
        cout << "\n---------------------------------------" << endl;
        cout << "\n[!] The number you entered was invalid." << endl;
        cout << "\nPlease re-enter a number from 1 to 3" << endl;
        cout << "-> ";
        cin >> input;
    }

    cout << "You chose " << input << endl;
}
moooeeeep
  • 31,622
  • 22
  • 98
  • 187
Rick_Sch
  • 456
  • 3
  • 10
  • 19
  • Read `string` and http://stackoverflow.com/questions/2844817/how-do-i-check-if-a-c-string-is-an-int and http://stackoverflow.com/questions/7021725/converting-string-to-integer-c – Mateusz Nov 03 '12 at 18:21

4 Answers4

8

The problem is that:

cin >> input;

Will cause the bad bit to be set when you try and read a non numeric value. After that happens any attempt to use the operator>> is silently ignored.

So the way to correct for this is to test if the stream is in a good state and if not then reset the state flags and try and read again. But note that the bad input (that caused the problem) is still on the input so you need to make sure you throw it away as well.

if (cin >> input)
{
    // It worked (input is now in a good state)
}
else
{
    // input is in a bad state.
    // So first clear the state.
    cin.clear();

    // Now you must get rid of the bad input.
    // Personally I would just ignore the rest of the line
    cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

    // now that you have reset the stream you can go back and try and read again.
}

To prevent it getting stuck (which is caused by the bad bit being set) read into a string then use a string stream to parse user input. I also prefer this method (for user interactive input) as it allows for easier combination of different styles of reading (ie combining operator>> and std::getline() as you can use these on the stringstream).

#include <iostream>
#include <sstream>
#include <string>
// using namespace std;
// Try to stop using this.
// For anything other than a toy program it becomes a problem.

int main(int argc, char *argv[])
{
    int          input;

    std::string  line;
    while(std::getline(std::cin, line))   // read a line at a time for parsing.
    {
        std::stringstream linestream(line);
        if (!(linestream >> input))
        {
             // input was not a number
             // Error message and try again
             continue;
        }
        if ((input < 1) || (input > 3))
        {
             // Error out of range
             // Message and try again
             continue;
        }
        char errorTest;
        if (linestream >> errorTest)
        {
             // There was extra stuff on the same line.
             // ie sobody typed 2x<enter>
             // Error Message;
             continue;
        }

        // it worked perfectly.
        // The value is now in input.
        // So break out of the loop. 
        break;
    }
}
Martin York
  • 257,169
  • 86
  • 333
  • 562
  • `ctest.cpp: In function 'int main(int, char**)': ctest.cpp:14:42: error: variable 'std::stringstream linestream' has initializer but incomplete type ` – tuergeist Nov 03 '12 at 18:39
  • 1
    @tuergeist: Fixed. But you could have fixed it for me. – Martin York Nov 03 '12 at 18:42
  • `using namespace` is always ok in a cpp (especially in this case). It's inside a header that it will surely give you a headache. To avoid any collision though, I prefer to explicitly call what I will be using with `using theNameSpace::TheThingIneed;` and only inside cpp. Always be fully explicit inside a header file. – Emile Bergeron Feb 13 '14 at 01:55
  • 1
    @emileb: I would disagree. Read: [Why is “using namespace std;” considered bad practice?](http://stackoverflow.com/q/1452721/14065) The using construct is a problem in most situations and leads to the introduction of bugs when you maintain the code that are hard to spot and diagnose. – Martin York Feb 13 '14 at 19:14
  • 1
    @LokiAstari I'm aware of the possible problems that this may lead to, but these problems are limited to the inside of a cpp source file if you keep the "using" stuff outside of any header file. Still, this may lead to problems, but in the code of the OP, it would be useless to be fully explicit. – Emile Bergeron Feb 14 '14 at 21:50
  • 1
    @LokiAstari another thing is, with e.g. `using Foo::A;` and then, you add `using Bar::A`, it's easy to spot that you're making two class with the same name accessible in the same scope. Most of the time, you'll have your `using` lines at the same place in your cpp, making the possible errors easy to fix. Never ever use `using namespace A;` and `using namespace B;`, with two namespaces fully accessible like this, your asking for trouble. I'm giving you the point still, better to avoid if you don't know the risk. – Emile Bergeron Feb 14 '14 at 21:56
  • 1
    @emileb: Again I disagree `it would be useless to be fully explicit.`. The problem here is habit. If you don't get into the habit of doing it properly you end up with code where you do it automatically and thus make it hard to read. – Martin York Feb 17 '14 at 07:45
  • 1
    @emileb: Again I disagree: `Most of the time, you'll have your using lines at the same place` If you are putting them in the same place you are probably already doing it wrong. You should be containing the scope as tightly as possible. – Martin York Feb 17 '14 at 07:45
  • @LokiAstari I meant at the same place in a cpp file regarding a single class for example. And I'm talking about things like `using std::string;` or `using std::cout;`. These lines should be both at the top of the cpp file in which they are meant to be used. If you use another implementation of string inside that same cpp file, it's easy to see the `using` line at the top, and the errors that this could cause are limited to that cpp file. Maybe if you have very large project with very large cpp (couple thousand lines), I guess you'd be better to be fully explicit. – Emile Bergeron Feb 17 '14 at 22:58
  • 1
    @emileb: No. I think the `using` clause should be restricted to the smallest scope necessary when used (which means declaring inside the function where you need it). And doing it from `std::` objects seems like a waste of time at that point. – Martin York Feb 18 '14 at 01:31
  • Only got it to work with `(!(linestream >> input))`. – Mikael Sep 12 '17 at 01:02
  • 2
    @MikaelMello: Thanks. Fixed. – Martin York Sep 12 '17 at 02:17
2
#include <iostream>
#include <string>

using namespace std;

int validatedInput(int min = 1, int max = 3)
{
    while(true)
    {
        cout << "Enter a number: ";
        string s;
        getline(cin,s);
        char *endp = 0;
        int ret = strtol(s.c_str(),&endp,10);
        if(endp!=s.c_str() && !*endp && ret >= min && ret <= max)
            return ret;
        cout << "Invalid input. Allowed range: " << min << "-" << max <<endl;
    }
}

int main(int argc, char *argv[])
{
    int val = validatedInput();
    cout << "You entered " << val <<endl;
    return 0;
}
pokey909
  • 1,797
  • 1
  • 16
  • 22
1

Most of these answers include unnecessary complexity.

Input validation is a perfect time to use a do-while

do{

   cout << "\nPlease enter a number from 1 to 3:" << endl;
   cout << "-> ";

   if(!cin){
      cout << "Invalid input"
      cin.clear()
      cin.ignore(numeric_limits<streamsize>::max(), '\n');
   }

}while(!(cin >> input))
  • Use numeric_limits<streamsize>::max() to completely clear the buffer after a failed cin.

  • Use cin.clear() to reset the fail flag on cin so !cin wont always evaluate false.

cin.fail() is fine. However some would consider !cin more natural.

from my previous post https://stackoverflow.com/a/43421325/5890809

Community
  • 1
  • 1
Shan L
  • 81
  • 6
0

You declared input as int but when you write an alphanumeric character to input it will try to implicitly convert it into integer. But you error checking does not account for this.

Ur problem can be easily solved by changing your while loop. instead of checking this how about you check

while(input!=1 || input!=2 || input!=3)
RDismyname
  • 173
  • 1
  • 1
  • 10
  • 1
    Unfortunately not. The stream is in a bad state. You need to reset the state flags before retrying the read. – Martin York Nov 03 '12 at 18:30
  • What if we have to check if input is between 1 and 12000, or just validate input to see that the user enters an integer and not alphanumeric gibberish? – user0 Sep 30 '18 at 09:10