Why Switch Case Ends Up in an Infinite Loop When user Enters Letters? C/C++

Can you show all of your code, please ?

chars are internally treated as ints ... if you enter a char then your program will use the ASCII value of the char as input.

You might also see an issue if you listed your default case first and forgot the 'break' statement after it.  It might look like it isn't getting hit.

Here is my code. I got other switch cases, which got the same problem. All switch cases i write, it must have the mentioned problem when user enters value other than int. NOTE: if userChoice receives int, but if it receives char, this problem does not occur.

        do
	{
		choice_is_invalid = false;
		getUserChoice();
		switch(userChoice)
		{
			case 1:
				//do something
				break;
			case 2:
				//do something
				break;
			case 3:
				//do something
				break;
			case EXIT:
				exitApplication();
				break;
			default:
				cout<<"Choice is invalid."<<endl;
				choice_is_invalid = true; 
				continue;
		}
	}while(choice_is_invalid); 

Select allOpen in new window

It's hard to understand your English.  If you're parsing keyboard input, you probably need to use actual chars in your case statement.  Notice the quotes:

                        case '1':
                                //do something
                                break;
                        case '2':
                                //do something
                                break;
                        case '3':
                                //do something
                                break;
                        case EXIT:
                                exitApplication();
                                break;
                        default:
                                cout<<"Choice is invalid."<<endl;
                                choice_is_invalid = true;
                                continue;

How is userChoice defined ?
How is getUserChoice() implemented ?
What value is EXIT ?


>>                                 choice_is_invalid = true;
>>                                 continue;

If the choice is invalid, you set choice_is_invalid to true, because you want to continue the loop, but then you use continue; which basically skips the check at the end of the loop, and goes back to the beginning of the loop, where choice_is_valid is immediately reset to false. That's doing twice the same thing.
Remove the continue; (replace it with a break;) and see if that works better. If it doesn't, then please answer the questions I asked in the beginning of this post.

It seems I was not completely awake yet ;) Ignore the last paragraphs in my previous post, and replace them with :


>>                                 choice_is_invalid = true;
>>                                 continue;

If the choice is invalid, you set choice_is_invalid to true, because you want to continue the loop, but then you use continue; which basically skips to the check at the end of the loop, and then goes back to the beginning of the loop, where choice_is_valid is immediately reset to false.
Why is the continue; needed ? Is there something after the switch before the end of the loop ? Is it possible to post your entire code ?


Also, please answer the questions I asked in the beginning of my previous post.

All comments so far are correct (as far as I can tell), but after you showed the code no one has addressed your original question, which was that the default never is reached. Is the question inaccurate, do you only mean that things don't work quite as you intended?

With the code you sent, I can't see any way to leave the switch-statement without one of the cases being hit.

What if you try the attached modification and tell us exactly what output you got, that should clarify the problem for us?

do
	{
		choice_is_invalid = false;
		getUserChoice();
		switch(userChoice)
		{
			case 1:
				cout<<"Case 1."<<endl;
				break;
			case 2:
				cout<<"Case 2."<<endl;
				break;
			case 3:
				cout<<"Case 3."<<endl;
				break;
			case EXIT:
				cout<<"Case EXIT."<<endl;
				break;
			default:
				cout<<"Case default."<<endl;
				choice_is_invalid = true; 
				break;
		}
	}while(choice_is_invalid); 

Select allOpen in new window

1. userChoice is of type int in a private block.
2. check code below
3. const short EXIT = -1;

I tried the break; instead of continue, but the problem still there. I guess you are right, i do not need continue in default anyway, i put break instead.

If user type a letter, the letter will go to userChoice which expected to get int. After user press enter, i get an infinite loop as:

Enter a choice: Choice is invalid.

Enter a choice: Choice is invalid.

Enter a choice: Choice is invalid.

Enter a choice: Choice is invalid.

Enter a choice: Choice is invalid.

Enter a choice: Choice is invalid.

Enter a choice: Choice is invalid.

Enter a choice: Choice is invalid.

Enter a choice: Choice is invalid.

Enter a choice: Choice is invalid.

Enter a choice: Choice is invalid.

Enter a choice: Choice is invalid.

This problem always happens in switch case. Just make a quick switch statement that checks against int & put a letter instead int. You will fall into infinite loop if you use switch in a loop.

void MenusLayout::getUserChoice()
{
	cout<<"\nEnter a choice: ";
	cin>>userChoice;
}

Select allOpen in new window

>> If userChoice is an int and a non-int is entered, cin.fail will be true.

Moreover, the invalid character will stay on the stream. So, that's why you have your infinite loop.

I suggest using a more robust way of reading input, namely by using getline to read an entire line of input, and then parsing the data you need out of it (using a stringstream for example).

        http://www.cplusplus.com/reference/string/getline.html
        http://www.cplusplus.com/reference/iostream/stringstream/

Your answers gave me a good insight about the problem...THANKS

>>>> will read that same character again, fail again, read it again, etc.

No, there is no further read of the wrong char. The wrong state of the stream will cause each succeeding operation to fail again. You would have need to 'reset' the cin after fail to avoid the endless loop or use a string input - what probably is easier - which never fails.

This is a good one which solved the problem

 void MenusLayout::getUserChoice()
{
     string input;
     cout<<"\nEnter a choice: ";
     cin>>input;
     userChoice = atoi(input.c_str());
}

Try to do a cin.clear(), and you'll see that the character is still on the stream. The next iteration of the loop will read the same invalid character again, and fail again, and ...

It's true that the fail state needs to be reset, but that doesn't change the fact that the invalid character is still on the stream - and that's the only point I was making.

>> This is a good one which solved the problem

I would still suggest using getline to make the input line based.

And furthermore, I wouldn't use atoi, since there's no way to tell whether it succeeded or not. When the user gives invalid input (like an alphabetic character), atoi will simply return 0. And that might be a valid value.
I'd use a stringstream instead like I suggested earlier. The advantage is that you can use it the exact same way as you used cin, plus you can check the error flags etc.

i put cin.clear() & cin.ignore() in here, but didn't help

      default:
                                cout<<"Case default."<<endl;
                                choice_is_invalid = true;
                                //cin.clear();
                                break;

>>I would still suggest using getline to make the input line based.

i will use it and post the solution in here...

>> i put cin.clear() & cin.ignore() in here, but didn't help

That's because the invalid character is still on the stream as I said earlier ;)

If you use a more robust solution for input (like suggested earlier), you won't have these problems, and your input stream will always be in a valid state.

According to "Testing for extraction errors":

----------
Output error processing functions, discussed in Error Processing Functions, apply to input streams. Testing for errors during extraction is important. Consider this statement:

cin >> n;

If n is a signed integer, a value greater than 32,767 (the maximum allowed value, or MAX_INT) sets the stream's fail bit, and the cin object becomes unusable. All subsequent extractions result in an immediate return with no value stored.
-------------

If such a fail occurs, the fail-flag of cin is set and the character is not "read", it remains in cin as the next character.

So _without_ the cin.clear(), things go bad because cin is in failmode and any extraction fails immediately. With the cin.clear(), the failbit is cleared and the program tries once more to interpret the character as an int and sets the failflag again.

I think cin.flush() should work as you intended. But I do agree that using a string is better...

I just realized that the last line was a little unclear, maybe. More concret:

default:
   cout<<"Case default."<<endl;
   choice_is_invalid = true; 
   cin.clear();
   cin.flush();
   break;

Select allOpen in new window

>> I think cin.flush() should work as you intended.

No. flush is only for (buffered) output streams. Using it on cin (if your platform supports that) is not guaranteed to work.

here is the getline, i had to change userChoice to char....The program worked

But when i want to exit the application by entering -1, it goes to cout<<"Choice is invalid."<<endl;
instead of

case EXIT:
                 exitApplication();
                 break;

The reason is: i'm only extracting the first element. I will have to use a loop to extract the second element & that what i'm trying to avoid. However, i don't mind using a loop, but i'm trying to avoid using loops when there is no need for it. I got another solution, check it in the next post

void MenusLayout::getUserChoice()
{
	 string input;
         cout<<"\nEnter a choice: ";
         getline(cin, input);
	 userChoice = input[0];  //i took 0 element to avoid the rest
}

Select allOpen in new window

userChoice in this case is char...

What do you think of this solution?

The disadvantage: if user inserts -1, the case goes to default & application exits immediately which is wrong. The case should go to EXIT

 

        do
        {
                choice_is_invalid = false;
                getUserChoice();
                switch(userChoice)
                {
                        case '1':
                                //do something
                                break;
                        case '2':
                                //do something
                                break;
                        case '3':
                                //do something
                                break;
                        case EXIT:
                                exitApplication();
                                break;
                        default:
                                cout<<"Choice is invalid."<<endl;
                                choice_is_invalid = true; 
                                break;
                }
        }while(choice_is_invalid); 

Select allOpen in new window

Ok, here's an example of how you can use a stringstream to do this :

int getUserChoice() {
  int userChoice = -2;
  std::string input;
  std::cout << "Enter a choice : ";
  getline(std::cin, input);
  std::stringstream ss(input);
  ss >> userChoice;
  return userChoice;         /* -2 means that an invalid input was entered */
}

Select allOpen in new window

/* -2 means that an invalid input was entered */

you mean userChoice return -2 if user insert wrong datatype(non-int). Right?

int getUserChoice() {
  int userChoice = -2;
  std::string input;
  std::cout << "Enter a choice : ";
  getline(std::cin, input);
  std::stringstream ss(input);
  ss >> userChoice; //this part will override  int userChoice = -2;
  return userChoice;    /* -2 means that an invalid input was entered */
}

Select allOpen in new window

I will end it - i want to say THANKS to all those who have helped..

>> you mean userChoice return -2 if user insert wrong datatype(non-int). Right?

getUserChoice will return -2 if the user entered invalid data, yes.
Do test the function if you intend to use it - I quickly wrote it down ;)

i just tested it, i got two errors:

 error C2440: 'initializing' : cannot convert from 'std::string' to 'int'
warning C4552: '>>' : operator has no effect; expected operator with side-effect

i will have to do some conversions or castings.

The errors reside in here. Anyhow, i'll deal with it. I appreciate your help alot. In case i didn't use this method right now, it will help me in others parts of the code later on

stringstream ss(input);
ss >> userChoice;

Select allOpen in new window

It should compile fine. You did #include <sstream> and #include <string> didn't you ?

ops, i forgot <sstream>

Yes worked! Finally, phew.

Good job

I finally made a testprogram for this to give better advice, and the following works fine:

  int userChoice=-2;
  cin >> userChoice;
// userChoice is now -2 if an invalid number was entered.

So actually, going through a string is not necessary! If the user enters a letter, userChoice will stay -2 (you will _not_ get the ASCII value of the letter, as previously stated!).

>>>> atoi will simply return 0. And that might be a valid value.
No, it isn't.

The switch handled cases for (1,2,3,4,-1). That's why I used the atoi as it was the most simplest way to solve the issue.

I personally use getline and istringstream myself but I decided against here as I thought it would make the thing more difficult than needed ...

>> So actually, going through a string is not necessary!

It is, because it keeps the cin stream clean. If you wouldn't pass via a string, the fail state for the stream would be set, and the invalid data would stay on the stream, forcing you to clean that up before continuing. It's better to avoid that situation by using only getline for input.

>>> If the user enters a letter, userChoice will stay -2
But the cin stream still has fail state what will spoil any further user input ...

No, it isn't a good idea to let the cin fail ...

Sorry, you are right, of course! Which shows that even after creating a test program you might go into a trap :-)