4
\$\begingroup\$

Based on this question, I wrote my own implementation of the yesno function I suggested. The function reads an answer until it is either "y" or "yes" or "n" or "no" (case-insensitive).

#include <algorithm>
#include <cctype>
#include <cstdlib>
#include <iostream>
#include <string>

bool yesno(const std::string &prompt)
{
    while (true) {
        std::cout << prompt << " [y/n] ";

        std::string line;
        if (!std::getline(std::cin, line)) {
            std::cerr << "\n";
            std::cerr << "error: unexpected end of file\n";
            std::exit(EXIT_FAILURE);
        }

        std::transform(line.begin(), line.end(), line.begin(),
                       [](unsigned char x){return std::tolower(x);});

        if (line == "y" || line == "yes") {
            return true;
        }
        if (line == "n" || line == "no") {
            return false;
        }
    }
}

int main()
{
    bool include_digits = yesno("Should the password include digits?");
    if (include_digits) {
        std::cout << "le1mein\n";
    } else {
        std::cout << "letmein\n";
    }
}

I took care to:

  • Include all necessary headers.
  • Call the std::tolower function with an unsigned char as argument.
  • Use std::getline instead of the hard-to-control >> operator.
  • Catch all errors (except for writing to std::cout).

Anything that I missed?

\$\endgroup\$
3
  • \$\begingroup\$ My approach would be to create binary_response class and overload stream operators. Also I would overload operator bool, so the syntax and semantics would be dope af. Should I write this as answer? \$\endgroup\$ Commented Nov 20, 2017 at 21:42
  • \$\begingroup\$ @Incomputable It sounds a bit like overengineering to create a class and several operators instead of a simple function, but still it sounds interesting. I would definitely be interested in your answer. \$\endgroup\$ Commented Nov 20, 2017 at 21:45
  • \$\begingroup\$ I’ll try to post the answer with full implementation. I believe I should be able to create it tomorrow afternoon, which is around 10 hours from now. Unfortunately it is quite late at night, so my keyboard will definitely wake my roommates. \$\endgroup\$ Commented Nov 20, 2017 at 21:48

2 Answers 2

5
\$\begingroup\$

Good job in general, mad props for proper use of cout vs cerr.

I don't see any "errors" per se, but I do see a few things I personally would have done a bit differently.

Opinion: Not a big fan of std::exit() in anything but main()

A function is supposed to be decontextualized, and having a "will crash the whole program if it fails" as part of the function's contract is just too aggressive in my opinion.

Personally, I'd rather just throw an exception instead. It accomplishes the same thing, and it lets users of the function choose how to handle failures.

Opinion: maybe have a separate prompt for failure?

I would personally explain to the user why he is being asked the same question twice when the input fails to match either criteria.

Opinion: std::transform is overkill here

This is a personal bias, as I find in-place usage of std::transform uncomfortable at best, since a range-based for does as good a job in a much more legible manner.

Specifically: Having to read the third parameter to understand that it's an inplace transform is just unnecessary cognitive load. I find the following easier to parse:

for(auto& x : line) {
  x = std::tolower(static_cast<unsigned char>(x));
}
\$\endgroup\$
1
\$\begingroup\$

Overengineering

Well, as you've mentioned in the comments, my solution is a bit of overengineering.

Class

I'd write a bianry_response class.

class binary_response
{
    static std::unordered_set<std::string> positives;
    static std::unordered_set<std::string> negatives; //define both and give some defaults
    std::optional<bool> answer;
    std::string answer_str;
public:
    //rule of zero

    friend std::istream& operator>>(std::istream& is, binary_response& response)
    {
        is >> answer_str;
        if (positives.count(answer_str) == 1)
            {answer = true;}
        else if (negatives.count(answer_str) == 1)
            {answer = false;}
        else
        {
            is.setstate(std::ios::failbit); //standard behavior
            answer_str.clear();
        } 

        return is;
    }

    bool valid() const
    {
        return answer.has_value();
    }

    operator bool() const
    {
        return answer.value();
    }

    const std::string& literal_answer() const
    {return answer_str;}

    //manipulators for positives and negatives
    static void positives() {return positives;}
    static void positives(std::unordered_set<std::string> new_positives)
    {
        positives = std::move(new_positives);
    }

    static void negatives() {return negatives;}
    static void negatives(std::unordered_set<std::string> new_negatives) 
    {
        negatives = std::move(new_negatives);
    }

    //maybe something that would return default ones
}

Example

The above code would enable us to write the following:

binary_response response;
while (!(is >> response))
{
    os << "Please enter one of the variants shown above\n";
    is.ignore(std::numeric_limits<std::streamsize>::max());
}

return response; //will convert to bool automatically

Do note that operator>> is callable only using ADL, as it is not defined outside. Usually it should work as is, but some people might get confused from not seeing it in their IDE.

After trying to parse darn CSV data files I discovered some of the awesome features of iostreams. It is unfortunate that very few people care about it. The above code would enable us to get multiple responses in a row using std::copy(), for example (not sure if it is a useful feature). Although we teach how to overload operator>> here, we don't really mention that one should set failbit if the extraction fails for whatever reason.

Design decisions

  • Sets enforce uniqueness of valid responses in each category

    I don't think that having duplicates are bad, but enforcement comes almost for free, or even improves things. Though one can make a text that belongs to both, but I believe it is hard to do by accident.

  • Shared pool of valid responses

    I wasn't sure if local pool of responses would be useful. It would create very big memory overhead for such a simple task, but since the class itself is overkill, it is quite arguable.

  • Standard compliant streaming operator overload

    By far not all people know internals of iostream, but the idiomatic loop while (is >> variable) is quite widespread and well understood, so I thought that not having to learn something new is a good thing. Practically, the only thing they need to know is the name of the class. Everything else is intuitive (I'm sure IDE will help them out with that). This also provides fine grained control over the stream, and lets users to ignore/unget themselves.

  • Can only be extracted from the stream

    This is one of the "easy to use correctly, hard to use incorrectly". Copying into a local string, then passing into a function doesn't seem correct to me. People can always make std::istringstream from a string.

\$\endgroup\$
1
  • \$\begingroup\$ I'm not sure if it works correctly, unfortunately I don't have time to test it. \$\endgroup\$ Commented Nov 21, 2017 at 17:10

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.