CODEWITHSUNDEEP
c++if-statementlogical-operators
Stephen R. Hayes
Stephen R. Hayes

Reputation: 25

How to use the or statement (||) correctly, in an else-if statement

Consider this code:

string GameExit;
bool GameChoiceGo = true;

while (GameChoiceGo == true)
{
    system("cls"); 
    cout << "\n  Are you sure you want to exit? (Yes or No)     ";
    cin >>  GameExit;
    if (GameExit == "y" || "Y" || "yes" || "Yes" || "YES")
    {
        cout << "User typed Yes";
        Sleep(3000);
        system("cls");
        break;
    }
    if (GameExit == "n" || "N" || "no" || "No" || "NO")
    {
        cout << "User typed No";
        Sleep(3000);
        system("cls");
        GameChoiceGo = false;
    }
    else
    {
        cout << "\nI'm sorry but, " << GameExit << " is not a acceptable choice. Type: Yes or No.\n\n\n";
        Sleep(3000);
        system("cls");
    }
}
break;

Here, only the first statement is activated. Even if the user types "No" or anything else, it will output "user typed yes".

The else-if statements work if I replace the or statements with only one statement (i.e. "y" and "n"). The only problem is, I want to have any possible version of yes and no that the user might type in the code.

Any ideas why the code is not working correctly?

Upvotes: 0

Views: 157

Answers (4)

erol yeniaras
erol yeniaras

Reputation: 3795

Correct way using OR operator in "your" code is as below (note the explicit use of == statements between || operators):

    if (GameExit == "y" || GameExit =="Y" || GameExit =="yes" || GameExit =="Yes" || GameExit =="YES")
    {
        cout << "User typed Yes";
        Sleep(3000);
        system("cls");
        break;
    }
    if (GameExit == "n" || GameExit =="N" || GameExit =="no" || GameExit =="No" || GameExit =="NO")
    {
        cout << "User typed No";
        Sleep(3000);
        system("cls");
        GameChoiceGo = false;
    }

PS: The above answer is not intended to give the best programming practice in a similar situation but to give the specific answer to the OP with the minimal code change :)

// -----

EDIT: Here is a better approach using STL. Note that (unsorted) array lookup requires linear search, whereas unordered_set, which is a hash set, has (on average) constant time lookup. This will be faster especially when the yes, no etc. options are plenty.

    #include <unordered_set>

    ...
    // These sets can be as large as possible or even dynamically 
    //  updated while the program is running. insert, remove, lookup will
    // all be much faster than a simple array.
    unordered_set<string> ySet{"y", "Y", "yes", "Yes", "YES"};
    unordered_set<string> nSet{"n", "N", "no", "No", "NO"};

    if (ySet.find(GameExit) != ySet.end())
    {
        cout << "User typed Yes";
        Sleep(3000);
        system("cls");
        break;
    }
    if (nSet.find(GameExit) != nSet.end())
    {
        cout << "User typed No";
        Sleep(3000);
        system("cls");
        GameChoiceGo = false;
    }
    ...

Upvotes: 4

Rakete1111
Rakete1111

Reputation: 48948

I'm sorry, but you have to write GameExit == for every condition you want to check:

if (GameExit == "y" || GameExit == "Y" || GameExit == "yes" || GameExit == "Yes" || GameExit == "YES")

If you write if ("y") (which is basically what you are doing, only with more statements), the const char[] will decay to a const char*, and that pointer will be compared to 0. Now, that pointer will never be null, as there will always be memory allocated for the string literal.

A better solution is to (1) make an array with all the options, so that checking the conditions becomes a simple search or (2) convert the input to all lowercase for example, and compare that.

// 1)
std::vector<std::string> options = { "y", "Y", "yes", "Yes", "YES" };

if (std::find(options.begin(), options.end(), GameExit) != options.end());
// or
if (std::any_of(options.begin(), options.end(), [&GameExit](const auto& value) { 
        return GameExit == value;
   }); 

// 2)
std::transform(GameExit.begin(), GameExit.end(), GameExit.begin(), ::tolower);

if (GameExit == "y" || GameExit == "yes");

You can look up the functions if you do not know what they do :).

Upvotes: 8

mreff555
mreff555

Reputation: 1111

You need to define a complete equality for each expression like this:

if ( gameExit == "y" || gameExit == "Y" ) {}

Upvotes: 3

Zackary Murphy
Zackary Murphy

Reputation: 411

GameExit == "y" || "Y" || ....

is incorrect. The correct method is:

GameExit == "y" || GameExit == "Y" || ....

and so on, both for the yes or no case.

Upvotes: 1

Related Questions