If statement causing loop to not fully run? C++

Question

for (int k = 0; k != s.size(); k++)
{
    if (s[k] == 'A' || s[k] == 'B' || s[k] == 'C' || s[k] == 'D' )
        return true; 
    else 
        return false;
}

I am trying to check to see if the entered string contains any characters other than these. If it does, I want it to return false. However, When I put in a string like "ABCEDF" it returns true. It seems like it's only checking the first character/index. How can I use for loops and if statements to check if the entered string meets the correct criteria and returns true/false accordingly?

Answer 1

The problem is that you are really checking whether the very first char is one of those listed in the if. The error is algorithmic because in order to decide whether the string contains chars other than ABCD you have to at least read every single char of the input string. This translates to: return true inside the loop if you read anything different from ABCD. If you have read the whole string and you still have not returned, well, guess what, the string is only made of ABCDs so you can return false!

Plus, if you want to check whether the string contains any char other than ABCD you should check whether s[k]!=A && s[k]!=B && s[k]!=C && s[k]!=D

Something, as follows, should work:

bool otherThanABCD(const string& s){
    for (int k = 0; k != s.size(); k++){
        if (!(s[k] == 'A' || s[k] == 'B' || s[k] == 'C' || s[k] == 'D') )    
    // there is a char which is not ABC or D!
          return true; 
   }
    // I have read the whole string, but still nothing other than ABC and D
 return false;
}

Answer 2

You really do not need loops and a bunch of if statements to do this task you can write a simple function that'll do this for you. Check this bit of code out!

#include <string>
#include <iostream>

// This checkString function is all you really need!
bool checkString( const std::string& stringToCheck, const std::string& checkAgainstThis ) {
    if ( stringToCheck.empty() || checkAgainstThis.empty() ) {
        // throw exception - both strings have nothing in them!
    }

    std::basic_string<char>::size_type firstIndex = stringToCheck.find_first_not_of( checkAgainstThis );
    if ( firstIndex == std::string::npos ) {
        return true;  // Didn't find anything.
    }

    return false;  // Found something other than.
}

// helper functions to display messages in main function - these are not required for
// the above function; just makes the main function cleaner and easier to read

void displayCheckingMessage( const std::string& a, const std::string& b ) {
    std::cout << "Checking string " << a << "\nto see if it contains any character other than this set: " << b << ".\n";
}

void displayResultMessage( bool condition, const std::string& str ) {
    if ( condition ) {
        std::cout << "Did not find any other characters.\n\n";
    } else {
        std::cout << "Found something other than " << str << ".\n\n";
    }
}

int main() {    
    std::string stringToCheck( "ABCADCDABC" );
    const std::string checkAgainstThis( "ABCD" );

    displayCheckingMessage( stringToCheck, checkAgainstThis );
    displayResultMessage( checkString( stringToCheck, checkAgainstThis ), checkAgainstThis );

    stringToCheck.clear();
    stringToCheck = std::string( "ABCDE" );
    displayCheckingMessage( stringToCheck, checkAgainstThis );
    displayResultMessage( checkString( stringToCheck, checkAgainstThis ), checkAgainstThis );

    stringToCheck.clear();
    stringToCheck = std::string( "AAABBBCCCDDDAAABBDDCCSSRRAASSGGSS" );
    displayCheckingMessage( stringToCheck, checkAgainstThis );
    displayResultMessage( checkString( stringToCheck, checkAgainstThis ), checkAgainstThis );

    stringToCheck.clear();
    stringToCheck = std::string( "AAAAAAAAAAAAAAAAABBBBBBBBBBBBBAAAAAAAAAAAAAAACCCCCCCCCCCCDDDDDDDDDDD" );
    displayCheckingMessage( stringToCheck, checkAgainstThis );
    displayResultMessage( checkString( stringToCheck, checkAgainstThis ), checkAgainstThis );

    stringToCheck.append( "XABCD" );
    displayCheckingMessage( stringToCheck, checkAgainstThis );
    displayResultMessage( checkString( stringToCheck, checkAgainstThis ), checkAgainstThis );

    // try a different set
    std::cout << "\n";
    const std::string checkAgainstThis2( "XYZ" );
    displayCheckingMessage( stringToCheck, checkAgainstThis2 );
    displayResultMessage( checkString( stringToCheck, checkAgainstThis2 ), checkAgainstThis2 );

    stringToCheck.clear();
    stringToCheck = std::string( "XYZXYZXYZXYZ" );
    displayCheckingMessage( stringToCheck, checkAgainstThis2 );
    displayResultMessage( checkString( stringToCheck, checkAgainstThis2 ), checkAgainstThis2 );

    stringToCheck.append( "8?#XYZXYSAZC" );
    std::cout << "\n" << stringToCheck << "\n";
    displayCheckingMessage( stringToCheck, checkAgainstThis2 );
    displayResultMessage( checkString( stringToCheck, checkAgainstThis2 ), checkAgainstThis2 );

    return 0;
}

With this simple function you can check any first string to see if it contains any character other than the set of characters you want to check against from the second string. This also prevents you from having to "hard code" characters in if statement conditionals. This also gives you the added bonus to compare any string against a set of determined characters.

Answer 3

If the condition is true you return true. If the condition is false you return false. There is no branch in the condition where you do not return.

Your code is equivalent to

for (int k = 0; k != s.size(); k++)
{
    return (s[k] == 'A' || s[k] == 'B' || s[k] == 'C' || s[k] == 'D' );
}

In other words, you return unconditionally.

---

Without knowing exactly what you want to do or what you're doing, I guess you might want something like this instead:

for (int k = 0; k != s.size(); k++)
{
    if (s[k] == 'A' || s[k] == 'B' || s[k] == 'C' || s[k] == 'D' )
        return true;
}

return false;

Where you return false only after the loop.