Reading a number from the standard input

Question

This would seem obvious, but there is a lot that can go wrong. What happens is the user enters alpha values etc.

I have come up with this function and would appreciate any suggestions or comments on how to handle this better:

#include <iostream>
#include <string>
#include <cstdlib>
#include <limits>

bool readNumber(int& value, const std::string failPrompt = "")
{
    for (;;) 
    {
        std::cin >> value;
        if (std::cin.fail())
        {
            if (std::cin.eof())
                return false;
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
            if (failPrompt.empty())
                return false;
            std::cout << failPrompt;
            continue;
        }
        return true;
    }
}

int main() 
{
    std::cout << "Please enter a number.\n";
    int value;
    if (!readNumber(value, "That is not a number, please try again\n"))
        return EXIT_FAILURE;
    std::cout << "number = " << value << "\n";
}

Answer 1

Consider all possible errors

You only considered the possibility of failing to parse the number and reaching the end of the input. However, there might also be a read error that causes fail() to return true, eof() to return false, but which cannot be recovered from. That will cause readNumber() to go into an infinite loop. A solution is to check if bad() is true, and if so return false.

Another solution is to read in a whole line at a time using std::getline(), and parse the line that you read using std::stoi(), like so:

for (std::string line; std::getline(cin, line); std::cout << failPrompt) {
    std::size_t pos;
    value = std::stol(line, &pos);

    if (line[pos] == '\0') {
        /* The whole line was a valid number */
        return value;
    }

    /* Else there was a parse error */
}

Write error messages to std::cerr

You should prefer using std::cerr for error messages. This is especially useful if the normal output is redirected to a file for example.

Pass strings by reference when possible

Your readNumber() takes a string by value, which makes an unnecessary copy. Pass it by reference instead:

bool readNumber(int& value, const std::string& failPrompt = "")

Consider using std::optional to return the value

It's good that you return a bool, so it makes it easy to check for an error. You could mark the function [[nodiscard]] as well so the compiler will warn if the caller ignores the return value.

However, it still is annoying that you have to declare a variable first and then pass that by reference. It would be nice if you got the number that was read as the return value. Since C++17, there is a handy type that can give you both the number and indicate whether you succesfully read the input or not: std::optional. Your code would then look like:

std::optional<int> readNumber(const std::string &failPrompt = "")
{
    ...
    if (/* value read succesfully */)
        return value;
    else /* if there is an error */
        return std::nullopt;
}

And then you can use it like so in main():

if (auto value = readNumber("Try again\n"))
    std::cout << "number = " << *value << "\n";
else
    return EXIT_FAILURE;

Answer 2

You are still reading an int from cin, and the only error case you are taking care of is entering a blank line. The normal reader will stop when it sees a non-digit, leaving the rest to be read later, even though the code waited here until Enter was pressed.

For example, if the user typed 25xyz it would happily return 25 and not find any error. You're making a big fuss out of closing the standard input handle (?!) and really only allowing him to enter blank lines before the real answer.

What such a function needs to do is read the whole line as a string, e.g. with getline. Then parse the line and make sure it contains the number and nothing else as opposed to stopping when it sees a non-digit. The newer functions do that naturally and are faster, so use from_chars to do the conversion.

---

There are several issues with the form of the function itself, too.

bool readNumber(int& value, const std::string failPrompt = "")

First, you are using an "out" parameter, which is one of the annoying issues with standard I/O and something we preach against in Code Reviews. We want the user to be able to write const auto years = readNumber(); in the "nice" way that all variables should be defined.

Similarly, you are passing a std::string by value ? You know that's a big beginner mistake. The default value of an empty string is created by calling the const char* constructor which is inefficient since it handles a more general case. The proper, efficient and idiomatic way to specify an empty string is with {}.

But, it is also best practice to pass this as a string_view, so it can efficiently take a lexical string literal without copying it, or an existing std::string.

---

Your behavior of returning false or however we decide to fail if there is no prompt given means that many many uses will use a canned generic fail string like the one in your example. For an interactive program, it should normally retry with a message explaining the input needed. I don't know how you can customize it meaningfully; nothing about the caller changes what input this function considers legal.

design goals

I'm thinking this is meant for use in simple interactive programming exercises and student code. So, it should be simple and does not need lots of configuration options and flexibility. It reads from standard input, period.

It should promote good programming practices, and not be "different" because I/O streams are different.

I imagine something like this:

const auto age = input<int>("What is your age in years?", between(1,999));

The prompt can be integrated into the same call, which is not only handy to use, but facilitates implementations that are fancier than just dumb TTY. The prompt can be repeated after an error, or the cursor repositioned in the correct spot, or it could be a pop-up form, etc.

Doing the validation in the same call is important because that is where it is doing the retry and not returning until it gets something it likes. If you know you got a valid number but still need to check the range, it makes the user code some kind of retry capability himself anyway.

Note that I used a named constraint rather than just two more parameters. This makes it clear what the numbers mean, and allows for more kinds of constraints. In fact, between can be an object and this uses the same form as the more general one that takes a lambda.

Note that the resulting value is "do or die" and can be used to initialize a const variable.

There may be cases where entering something is optional, but that is not typical of these little problems, and should be done with a different call that uses a sum type like optional. In fact, the template should be written to just take optional<T> as being an empty string or the normal parsing work for T.

const auto old_score= input<std::optional<int>>("Enter your previous high score, if any:");

Sometimes you might have an existing value that can be used as a default, so a blank line takes that default. This could use a different function that takes an in/out parameter, say:

int block_size = 32768;
   ⋮
edit(block_size, "block size");

The function would automatically show the default value and format the prompt.

Or, it could use additional parameters to enable a default value:

const auto block_size = input<int>("block size", Default(32768));

But this has issues. The word default is a reserved word already. Having multiple different optional arguments is a complex issue in itself that we can avoid. And, the prompt is formatted automatically with the default value and punctuation, so we want to give it a simple label rather than a sentence. That alone makes me thing it should be a different function.

good enough?

Really, what I want for this kind of utility program, also suitable for student program exercises, is a whole form of input handled in a unit, not just one line at a time. These programs should normally take input via command line arguments, but prompt if run interactively. So, the arguments should be described once and used for both cases.

But a single-value BASIC-like input statement that works better than standard input for interactive questions and can be used to initialize const variables in the "best practices" manner would be far better than just letting the poor beginner get distracted by this before even getting to the real code.

Even if it's not great, by giving them this function we indicate that it's not their problem.

Answer 3

Consider making this a template function, so it can read values of types other than just int.

And all the things G. Sliepen recommends.

Answer 4

If this is interactive user input. I would read the whole line into a string and then parse that.

Note: This function expects one input item per line. This is what you would expect when interacting directly with a human; as the standard input is buffered until a new line. BUT when interacting with machine input this may not be the expected format and all input items could be placed on the same line.

template<typename T>
bool readInteractiveValue(T& value, std::string const& message)
{
    // Works for symmetric types that have input on 1 line.
    // Note: std::string is not symmetric as >> only reads a word.
    for(;;)
    {
         std::string   line;
         if (!(std::getline(std::cin, line)))
         {
              // If you can't read a line then something
              // very bad has happened and you can't continue.
              return false;
         }

         // We have some user input.
         std::stringstream linestream(std::move(line));

         // I like to use a temp value.
         // If the read fails I don't want to alter the original.
         // So read into a temp value then swap if it all works.
         T tempValue;
         if (linestream >> tempValue)
         {
              // The user input was good.
              // But let us check there is no extra cruft on the line.
              // e.g.  reading an integer and getting   10K
              //       The K should render this bad input.
              // So if we try and read and character and succeed then
              // the input is actually bad.
              char x;
              if (linestream >> x)
              {
                  // We found cruft. So input is bad.
              }
              else
              {
                  // input was good so let us return.
                  using std::swap;
                  swap(tempValue, value);
                  return true;
              }
         }
         // If we get here there was an error.
         std::cout << message << "\n";
         std::cout << "Try Again\n";
    }
}