Calling boolean function in while loop

Question

I have a bool function with an if else statements inside it. The first 'if' returns 'false' and the else returns 'true'. I want to call this boolean function in an other function with a while loop inside it. I have tried several times and I made it work. However, when I call the function it return 'true' all the time. How can I fix that?

bool secretCheck(string P1_name, string P2_name, char secret){
    secret =  'R', 'G', 'B', 'P', 'Y', 'M';
    if (secret != 'R' && secret != 'G' && secret != 'B' && secret != 'P' && secret != 'Y' && secret != 'M' && secret > 4){
        return false;   
    }
    else {
        return true;
    }
}

void secretLoop(string P1_name, string P2_name, char secret){
    while(!secretCheck(P1_name, P2_name, secret)){
        cout << "Invalid secret!"<< endl;
        cout << P1_name << ", please enter your secret: ";
        cin >> secret;
    }

    if(secretCheck(P1_name, P2_name, secret)) {
        cout << "Ok\n";
    }
}

Why do you have the line `secret = 'R', 'G', 'B', 'P', 'Y', 'M';`? What do you think it does? — aschepler, Mar 28 '19 at 17:35
I think it stores the characters that secret can take. doesn't it? — Yassine Ezzaim, Mar 28 '19 at 17:35
@YassineEzzaim No. I'm guessing if you had warnings enabled you'd see some related to that line. — Random Davis, Mar 28 '19 at 17:36
@aschepler I did not have any warnings :/, is that line wrong? how can I store a list of characters in that char? — Yassine Ezzaim, Mar 28 '19 at 17:38
@YassineEzzaim you cannot have a list of character in a char. You can have a list of character, or you can have a char. A variable cannot have multiple types at the same time — Guillaume Racicot, Mar 28 '19 at 17:41
@YassineEzzaim Must read: https://stackoverflow.com/questions/54142/how-does-the-comma-operator-work — πάντα ῥεῖ, Mar 28 '19 at 17:43

Guillaume Racicot · Answer 1 · 2019-03-28T17:51:39.680

1

I'm unsure what you're trying to do with this line:

secret =  'R', 'G', 'B', 'P', 'Y', 'M';

A char can hold one and only one value. In this case, you're using the comma operator which drops the first value and returns the second expression.

At that line, secret is always equal to 'R', since it's equivalent to say:

secret = 'R'; // now secret is equal to the character 'R'

Then, in your if statement, you got that:

secret != 'R' && /* ... */

This will always be false, since secret is always equal to 'R'.

Then, there's a boguous comparison:

secret > 4

This is also true, since comparing a char and a int will compare the ASCII value of the character.

To store multiple characters in a variable it will either have to be of type std::string or std::vector:

std::vector<char> secret_characters = {'R', 'G', 'B', 'P', 'Y', 'M'};

And if you want to check if secret is in the secret_character list, you can do that:

void is_secret(std::vector<char> const& secret_characters, char secret) {
    auto found = std::find(
        secret_characters.begin(), secret_characters.end(),
        secret
    );
    return found != secret_characters.end();
}

bool secretCheck(string P1_name, string P2_name, char secret){
    std::vector<char> secret_characters = {'R', 'G', 'B', 'P', 'Y', 'M'};
    if (is_secret(secret_characters, secret)) {
        return false;   
    } else {
        return true;
    }
}

edited Mar 28 '19 at 17:51

answered Mar 28 '19 at 17:38

Guillaume Racicot

39,621
9
77
141

The comma operator has lower precedence than assignment, so `secret` is actually set to `'R'`. Same problem, though. – aschepler Mar 28 '19 at 17:41
How can I assign those characters to a variable type char or string ? – Yassine Ezzaim Mar 28 '19 at 17:42
@YassineEzzaim I have a hard time understanding what you're trying to ask me. You want a list of character or a single character? – Guillaume Racicot Mar 28 '19 at 17:43
I want to check if Secret does include only and only those letters when the user Input the letters. – Yassine Ezzaim Mar 28 '19 at 17:45
@YassineEzzaim You don't particularly need a list of characters here. You could just start `secretCheck` immediately with `if (secret != 'R' &&` ... `&& secret != 'M')` Though something like `string::find` would be a different way to do it. – aschepler Mar 28 '19 at 17:46
@YassineEzzaim ok, got it. Be aware that the user can only input a single letter, since `secret` is a `char` (a single character) – Guillaume Racicot Mar 28 '19 at 17:47
@ascheple oh thank you so much for your feedback I will fix that. – Yassine Ezzaim Mar 28 '19 at 17:47
@GuillaumeRacicot I really appreciate your help ! – Yassine Ezzaim Mar 28 '19 at 17:48

Will Eccles · Answer 2 · 2019-03-28T17:42:51.053

In your function secretCheck, you assign the value 'R' to secret, completely overwriting whatever it used to be. Then, you return false iff secret != 'R'. Thus, you will never return false, since secret will always be 'R'. It's unclear what you were trying to do in the first line of that function, especially since the meaning of the function is unclear (you never even use either string argument). I would suggest making sure your function is actually performing the task you want it to.

Calling boolean function in while loop

2 Answers2