String and character comparison-CodePudding

I am new to programming and I need to search any string to see if it includes only the letters a,b,c,d,e or f. The minute the program finds a letter that is not one of those the program should return false. Here is my function

bool is_favorite(string word){
    int length = word.length(); // "word" is the string.
    int index = 0;
    while (index < length) {
        if ((word[index] == 'a') || (word[index] == 'b') || (word[index] == 'c')||
             (word[index] == 'd')|| (word[index] == 'e')|| (word[index] == 'f')) {
            return true;
        }
        else {
            return false;
        }
        index  ;
    }
}

Thank you very much for nay help! :)

CodePudding user response：

The moment the return statement is encountered, the function is exited. This means that the moment any of the characters 'a', 'b', 'c', 'd', 'e', 'f' is encountered while iterating, due to the return statement the function will be exited immediately.

You can use std::string::find_first_not_of as shown below:

std::string input = "somearbitrarystring";
std::string validChars = "abcdef";
std::size_t found = input.find_first_not_of(validChars);
if(found != std::string::npos)
    std::cout << "Found nonfavorite character " <<input[found]<<" at position "<<found<< std::endl;
else 
{
    std::cout<<"Only favorite characters found"<<std::endl;
}

CodePudding user response：

If you unroll the loop by hand, you will spot the problem immediately:

if ((word[0] == 'a') || (word[0] == 'b') || (word[0] == 'c')||
    (word[0] == 'd')|| (word[0] == 'e')|| (word[0] == 'f')) {
    return true;
}
else {
    return false;
}
if ((word[1] == 'a') || (word[1] == 'b') || (word[1] == 'c')||
    (word[1] == 'd')|| (word[1] == 'e')|| (word[1] == 'f')) {
    return true;
}
else {
    return false;
}
//...

That is, the return value depends only on the first element.

"The minute the program finds a letter that is not one of those the program should return false" means

if ((word[0] != 'a') || (word[0] != 'b') || (word[0] != 'c')||
    (word[0] != 'd')|| (word[0] != 'e')|| (word[0] != 'f')) {
    return false;
}
if ((word[1] != 'a') || (word[1] != 'b') || (word[1] != 'c')||
    (word[1] != 'd')|| (word[1] != 'e')|| (word[1] != 'f')) {
    return false;
}
// ...
// After checking all the characters, you know what all them were in 
// your desired set, so you can return unconditionally.
return true;

or, with a loop:

while (index < length) {
    if ((word[index] != 'a') || (word[index] != 'b') || (word[index] != 'c')||
         (word[index] != 'd')|| (word[index] != 'e')|| (word[index] != 'f')) {
        return false;
    }
    index  ;
}
return true;

CodePudding user response：

bool is_favorite(string word){
    return ( word.find_first_not_of( "abcdef" ) == std::string::npos );
}

It returns true if, and only if, there are only the characters 'a' through 'f' in the string. Any other character ends the search immediately.

And if you exchange string word with const string & word, your function will not have to create a copy of each word you pass to it, but work on a read-only reference to it, improving efficiency.

CodePudding user response：

bool is_favorite(string word){
    int length = word.length(); // "word" is the string.
    int index = 0;
    while (index < length) {
        if (word[index] > 'f' || word[index] < 'a') 
            return false;

        index  ;
    }
    return true;
}

CodePudding user response：

The return true is logically in the wrong place in your code.

Your version returns true as soon as it finds one letter that is a through f. It's premature to conclude that the whole string is valid at that point, because there may yet be an invalid character later in the string.

bool is_favorite(string word){
    int length = word.length(); // "word" is the string.
    int index = 0;
    while (index < length) {
        if ((word[index] == 'a') || (word[index] == 'b') || (word[index] == 'c')||
             (word[index] == 'd')|| (word[index] == 'e')|| (word[index] == 'f')) {
            return true;  // This is premature.
        }
        else {
            return false;
        }
        index  ;
    }
}

Minimal change that illustrates where the return true should be: after the loop. The return true is reached only if and only if we did not detect any invalid characters in the loop.

bool is_favorite(string word){
    int length = word.length(); // "word" is the string.
    int index = 0;
    while (index < length) {
        if ((word[index] == 'a') || (word[index] == 'b') || (word[index] == 'c')||
             (word[index] == 'd')|| (word[index] == 'e')|| (word[index] == 'f')) {
            // Do nothing here
        }
        else {
            return false;
        }
        index  ;
    }
    return true;
}

Obviously now that the affirmative block of the if is empty, you could refactor a little and only check for the negative condition. The logic of it should read closely to the way you described the problem in words:

"The minute the program finds a letter that is not one of those the program should return false."

bool is_favorite(string word){
    int length = word.length(); // "word" is the string.
    int index = 0;
    while (index < length) {
        if (!is_letter_a_through_f((word[index])
             return false;
        index  ;
    }
    return true;
}

I replaced your large logical check against many characters with a function in the above code to make it more readable. I trust you do that without difficulty. My own preference is to keep statements short so that they are readable, and so that when you read the code, you can hold in your short-term memory the logic of what you are saying about control flow without being overloaded by the mechanics of your letter comparison.