23
\$\begingroup\$

I intend to accept all of the following answers:

  • y
  • Y
  • yes
  • YES
  • Yes
  • plus mixtures, such as yeS, yES, which I didn't intend to accept but it's a consequence of my code
  • the same applies to No and its variants

#include <iostream>     // std::cout, std::cin
#include <string>       // std::string
#include <algorithm>    // std::transform

int main()
{

    bool answer_valid = false;
    std::string answer;

    do
    {
        std::cout << "Should the password include digits? [Y/n] ";
        std::cin >> answer;

        std::transform(answer.begin(), answer.end(), answer.begin(), ::tolower);

        answer_valid =
            (answer == "y") ||
            (answer == "n") ||
            (answer == "yes") ||
            (answer == "no");

    } while (!answer_valid);

    return 0;

}

It works flawlessly, or it seems so. Is there better approach?

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I would simply defer it to regex library in a case like this. It is simple to improve and extend. Also, it is safer if your reviewer/manager knows regex. He can validate it since it is somewhat standardized. \$\endgroup\$ Commented Nov 23, 2017 at 19:19

6 Answers 6

22
\$\begingroup\$

You have a common bug that will send you into an infinite loop. :-)

When reading data (especially user input data) you must validate the read worked correctly. If the read fails and sets one of the error flags on the stream than any subsequent attempt to read will silently be ignored.

Thus usually read operations are checked in the loop test.

std::string word;
while(word != "STOP") {
    std::cin >> word;
    std::cout << "Word: " << word << "\n";
}

In the above you will enter an infinite loop if your hit an EOF marker (Note: this can be generated from the keyboard) or in unix systems a file can be piped to the input stream of an application etc..

To prevent this you test the result of the read operation.

std::string word;
while(std::cin >> word && word != "STOP") {
    std::cout << "Word: " << word << "\n";
}

This works because of two things. The operator>> returns a reference to the stream (usually for chaining). When a stream is used in a boolean context (like a test) it is converted to a bool using operator bool which calls good() which returns true if the stream is still in a good state (ie the read operation did not cause an error).

Secondly declares variables as close to the use point as possible.

This is a good habit to get into with C++ as objects have constructors that can potentially be expensive (also destructors). Declaring them at the top of the function and then not using them is wasteful.

Also be declaring them near the use point also allows for locality of reading when checking variables types.

In your case:

    std::string answer;

This is declared at the top but only used inside the loop.

Also prefer not to use return 0; in main() when the application never returns an error state. The compiler will automatically generate the required return 0 and leaving it off is a marker used by programs to indicate that the program never fails.

Give this I would rewrite as:

bool checkAnswerOK(std::string& answer, bool& result)
{
    std::transform(answer.begin(), answer.end(), answer.begin(),
                   [](unsigned char x){return ::tolower(x);});

     bool answer_valid =
            (answer == "y")   ||
            (answer == "n")   ||
            (answer == "yes") ||
            (answer == "no");

    result = answer_valid && answer[0] == 'y';    
    return answer_valid;
}


bool question_yesno(std::string const& message)
{
    std::string answer;
    bool        result;

    std::cout << message << "? [Y/n]\n";
    while(std::cin >> answer && !checkAnswerOK(answer, result))
    {
        std::cout << "Invalid answer: " << answer << " Please try again\n"
                  << message << "? [Y/n]\n";
    }
    if (!std::cin) {
        // We never got an answer.
        // Not much we can do here. Probably give up?
        throw std::runtime_error("User Input read failed");
    }
    return result;
}

int main()
{
    question_yesno("Should the password include digits");
}
\$\endgroup\$
5
  • 2
    \$\begingroup\$ will silently be ignored very nice API design... \$\endgroup\$
    – usr
    Commented Nov 20, 2017 at 21:42
  • \$\begingroup\$ @usr By design. So when you get to a place that you want to check things you can validate if it worked or not. You can of course change that behavior to much more noisy (throw an exception on error) but when dealing with user input this is less desirable behavior. \$\endgroup\$ Commented Nov 20, 2017 at 22:02
  • \$\begingroup\$ Usually you need to consume the result of a read immediately. Therefore you need to check immediately for errors. This question is an example of that. Exceptions would be suited very nicely for this. Altering instance state and needing to query the object to find out about errors seems like an unnecessary complication. We are not talking about invalid input. We are talking about a failed IO operation. \$\endgroup\$
    – usr
    Commented Nov 21, 2017 at 13:27
  • 2
    \$\begingroup\$ @usr Which is why we don't use Exceptions (Exceptions is a terrible mechanism for handling user error). Exceptions are designed for situations where you don't expect an error and you are passing that error up to a higher context to be handled by an unknown system manager. User errors (are expected) and thus handled at the point where they are generated and thus much better handled by error codes. \$\endgroup\$ Commented Nov 21, 2017 at 18:51
  • \$\begingroup\$ @usr I point you at Sutters Mill to go read about the topic. I am sure he has plenty of articles to cover the subject more thoroughly than we can do in comments. \$\endgroup\$ Commented Nov 21, 2017 at 18:54
20
\$\begingroup\$

For a beginner, that's very good code.

There is one thing that almost all C++ programmers get wrong, and that's calling ::tolower with a char argument. This leads to undefined behavior as soon as you enter German umlauts or café. Your program may appear to work, but it is not guaranteed to do so. Any good tutorial on the tolower function (which requires #include <ctype.h>, by the way) should mention that its argument must be cast to unsigned char.

For now, let the code as it is, but keep that in mind when you write code that has to accept untrusted data.

The next useful step would be to extract this code into its own function. Imagine how easy your code would look like if you could already call yesno with a prompt:

bool include_digits = yesno("Should the password include digits?");

To make this code work, you have to define the function like this:

bool yesno(const std::string &prompt) {
    // Your code here
    return valid_answer;
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ I must have been in a good mood when I wrote the above text. Now that I look at the code again, I see that I was just impressed that the code was not using namespace std. The code still leaves much room to improve. See codereview.stackexchange.com/q/180907 for an improved version of the code. \$\endgroup\$ Commented Nov 20, 2017 at 21:34
2
\$\begingroup\$

The existing reviews cover the most important aspects, I think, but as a supplement, I thought I'd show how it might be done with std::regex_match

#include <string>
#include <regex>
#include <iostream>

/* return 'y' if the answer matches the first regex,
 *        'n' if the answer matches the second regex,
 *        '\0' if the answer matches neither
 */
char yesno(const std::string &answer, const std::regex &yes, const std::regex &no) {
    if (std::regex_match(answer, yes)) {
        return 'y';
    } else if (std::regex_match(answer, no)) {
        return 'n';
    }
    return '\0';
}

int main() {
#if ENGLISH
    static const std::regex yes{"y|yes", std::regex::icase | std::regex::optimize};
    static const std::regex no{"n|no", std::regex::icase | std::regex::optimize};
    static const std::string prompt{"Should the password include digits? [y/n] "};
#else
    static const std::regex yes{"j|ja", std::regex::icase | std::regex::optimize};
    static const std::regex no{"n|nein", std::regex::icase | std::regex::optimize};
    static const std::string prompt{"Soll das Passwort Ziffern enthalten? [j/n] "};
#endif
    for (char answer{'\0'}; answer == '\0'; ) {
        std::string response;
        std::cout << prompt;
        std::cin >> response;
        if (std::cin) {
            answer = yesno(response, yes, no);
            std::cout << "Answer was \"" << answer << "\"\n";
        }
    }
}

As you can see, the regex is not so hard to use and offers a great deal of flexibility. In this case, I've showed how one could support English or German versions as an example.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I would rather pull the strings from L10N library than hard code them. \$\endgroup\$ Commented Nov 21, 2017 at 0:26
  • 2
    \$\begingroup\$ @LokiAstari Sure, me too in real code, but that would have obscured the point I was trying to illustrate and the OP is a self-described beginner. \$\endgroup\$
    – Edward
    Commented Nov 21, 2017 at 0:37
1
\$\begingroup\$

Lots of "fancy" solutions, but I haven't seen anyone mention the simplest one: Just check whether the first character is y or n

std::cout << "Should the password include digits? [Y/n] ";
    std::cin >> answer;
    answer_valid =
        (answer[0] == 'y') ||
        (answer[0] == 'n') ||
        (answer[0] == 'Y') ||
        (answer[0] == 'N');

You can still convert to upper of lower to simplify even more...

Based on the "php" answer by @WesToleman below, you could use:

std::cout << "Should the password include digits? [Y/n] ";
std::cin >> answer;
switch(answer[0])
{
    case 'y':
    case 'Y':
    case 'n':
    case 'N': return true;
}
return false;

You could add default: return false; but I prefer this way. Pros: only evaluate answer[0] once.

\$\endgroup\$
3
  • \$\begingroup\$ Unfortunately this returns true on inputs such as Yowsers! \$\endgroup\$ Commented Nov 21, 2017 at 1:39
  • \$\begingroup\$ Yes it does - which is kind of in keeping with the original question plus mixtures, such as yeS, yES, which I didn't intend to accept but it's a consequence of my code - As I said it's the simple approach. \$\endgroup\$
    – John3136
    Commented Nov 21, 2017 at 1:40
  • 2
    \$\begingroup\$ This is the first thing I thought of, and I'm sure i've used various "proper" utilities that work this way. You're asking the user to supply [y/n] in the question, and so it shouldn't come as a surprise that entering a string starting with y gets accepted as such. In my opinion trying to match a complete set of specific strings is a bit over-engineered for this. \$\endgroup\$
    – USD Matt
    Commented Nov 21, 2017 at 12:53
0
\$\begingroup\$

Assuming you have a fairly recent c++ compiler (c++11 or later), you can do this with a bit less code. It also gives you more versatility over then answers you can accept:

#include <iostream> 
#include <string>  
#include <set>

    int main()
    {

        std::string answer;

        std::set<std::string> validAnswers = {"Y", "y", "yes", "N", "n", "no", "si", "oui"};

        do
        {
            std::cout << "Should the password include digits? [Y/n] ";
        } while ((std::cin >> answer) && validAnswers.find(answer) == validAnswers.end());

        return 0;

    }

If you want to get really fancy, you can even use this to sort out what you do next:

#include <iostream>     // std::cout, std::cin
#include <string>       // std::string
#include <set>

int main()
{

    std::string answer;
    std::set<std::string> yesAnswers = {"Y", "y", "yes", "si", "oui"};
    std::set<std::string> noAnswers = {"N", "n", "no"};
    std::set<std::string> validAnswers;
    validAnswers.insert(yesAnswers.begin(), yesAnswers.end());
    validAnswers.insert(noAnswers.begin(), noAnswers.end());

    do
    {
        std::cout << "Should the password include digits? [Y/n] ";
    } while ((std::cin >> answer) && validAnswers.find(answer) == validAnswers.end());

    if (yesAnswers.find(answer) != yesAnswers.end())
       std::cout << "do yes stuff" << std::endl;
    else if (noAnswers.find(answer) != noAnswers.end())
       std::cout << "do no stuff" << std::endl;
    else
       std::cout << "do anything that's left" << std::endl;

    return 0;

}
\$\endgroup\$
0
\$\begingroup\$

My apologies, the C++ switch case only works on primitive types. That will teach me for writing answers late at night on my phone.


This alternative approach is O(1) however it is overkill for a small set of words such as in your question because the constant is larger than the time taken to run a small if or statement.

bool validWord(const std::string& word)
{
    const static std::unordered_set<std::string> validWords({ "yes", "no", "y", "n" });
    return validWords.count(word) == 1;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.