User input validation that might be unbreakable for C++

Question

I am new to programming and have been having a lot of trouble with user input validation. Everything I found online I could break and I finally put this function together that appears to be unbreakable. It is for int and double specifically.

I am curious about how it might be critiqued by experienced programmers. How did I do? What can be improved? What are some issues I might run into? One issue I ran into was when I #include<Windows.h>, max() becomes an issue in this code, so I had to use #define NOMINMAX. I wonder if there is a way around that as well. I am using this code with Visual C++ in Visual Studio.

#include<iostream>
#include<string> 
#include<limits>
using namespace std;
template<class T>
void input(T& input, int low, int high, string message)
{
input = low - 1;
while (input < low || input > high)
{
 cout << message;
 while ((cin >> input).fail() || cin.peek() != '\n')
 {
 cin.clear();
 cin.ignore(numeric_limits<streamsize>::max(), '\n');
 cout << message;
 }
 cin.clear();
 cin.ignore(numeric_limits<streamsize>::max(), '\n');
}
}

• Vorac: Templates are an advanced language feature (some people advise never to use them). What happens if T is std::string? std::array? MyClass? If you want to support multiple return types, I advise for overloading + factoring the common code into a separate helper function.
• Caleth: @Vorac for types where low - 1 isn't available, there will be a (nowadays reasonably understandable) compile time error. Templates are not a boogeyman
• Vorac: @Caleth with the new generic lambdas, templates are a thing of the past (or for above-average programmers, who I am not).
• Caleth: @Vorac ? I still prefer template<class T> void input(T& input, int low, int high, string message) ... to auto input = [](auto& input, auto low, auto high, string message) ...
• Loki Astari: You have an infinite loop issue. Try and read an integer using the above. Then enter <ctrl-D> (End of stream marker). Your code will enter an infinite loop.

Answer 1

I see some things that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It especially should not be used within a header that will be included in other code, as with this code, so don't do that!

Reconsider the interface

The current interface is this:

void input(T& input, int low, int high, std::string message)

However, I'd suggest that it would be more flexible if it were like this instead:

void input(T& input, const T& low, const T& high, std::string message)

That would allow, for floating point inputs, to have floating point bounds.

Restrict the template type

You say in the description that it's intended for this to work with int and double specifically, but there's nothing in the code that enforces that. For instance, we could attempt to call this with type T being a class that implements the operators called for in the template. The current code would accept that, and maybe that's the intent. For a class like std::complex<double>, the result would just be a series of possibly perplexing error messages.

Instead, I would suggest that the template type could be explicitly required to only be an arithmetic type by using std::enable_if and std::is_arithmetic. Here's how I'd do that:

template<class T>
void input(typename std::enable_if<std::is_arithmetic<T>::value, T>::type &val, 
 const T& low, const T& high, std::string message)
{
 for (std::cout << message; 
 !(std::cin >> val) || val < low || val > high; 
 std::cout << message) 
 { 
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 }
}

The syntax may seem confusing, but essentially what it does is to restrict type T to only plain old types which are either integers or floating point.

Answer 2

1. The #define NOMINMAX issue is easy to fix. To do so we need to remove(or move internally to our input function) the using namespace std; call as this pollutes our global namespace with max/min calls. See this for why it is bad practice to include the namespace.

2. Seeing as we only want to output our message string and do not care about copying, it is best to pass this to our function as const& std::string

3. I would change the function signature to make it easier to use in client code. Instead of passing in a temp var for input, just let the function return type T.

4. The type of low and high should be of the same type as T, not always int. For types such as double and float the comparisons may not always return expected results and may also need to be compared with some small epsilon value.

5. Finally we do not wish to modify low or high and should also pass them by const.

The final function might then look something like

#include<iostream>
#include<string> 
#include<limits>
template<class T>
T ValidateInput(const T low, const T high, const std::string& message)
{
 using namespace std;
 T input = low - 1;
 while (input < low || input > high)
 {
 cout << message;
 while ((cin >> input).fail() || cin.peek() != '\n')
 {
 cin.clear();
 cin.ignore(numeric_limits<streamsize>::max(), '\n');
 cout << message;
 }
 cin.clear();
 cin.ignore(numeric_limits<streamsize>::max(), '\n');
 }
 return input;
}

• Caleth: Might want to flip to a do ... while loop and not initialise input, then it doesn't underflow if low is numeric_limits<T>::min()
• const_ref: Yes true. Ideally it is likely better to split into two methods from a separation of concerns point of view. One to generate our value and one to actually validate.

Answer 3

Alternative Solution

How about this?

template<class T>
T input(T low, T high, std::string const& message)
{
 std::string line;
 while (std::cin)
 {
 std::cout << message;
 std::getline(std::cin, line);
 std::stringstream lineStream(line);
 T result;
 char x;
 if (lineStream >> result && result >= low && result <= high && !(lineStream >> x))
 {
 // This body entered iff.
 // 1) We successfully read a T from the stream into `result`
 // 2) result >= low
 // 3) result <= hight
 // 4) There is only white space on the line after the result.
 return result;
 }
 }
 throw std::runtime_error("No more user input could not read a value");
}

I hate all the faffing around with the bad bits and the clunky handling of the rest of the line. Yes there is a slight possibility of inefficiency if there is a bad line that is exceptionally long. But under normal operations I would not worry.

When reading formatted input with operator>> efficiency is not usually your biggest concern. When you are reading User Input then its (efficiency) definitely not your biggest concern as the user types very slowly.

I would also say it is the easiest to read out of the three solutions.

Comments on original Code

Things that stand out in the original:

while ((cin >> input).fail() || cin.peek() != '\n')

That call to fail() is superfluous as streams are automatically converted to a boolean (using good()) if the stream is used in a boolean context.

Also simply using fail() is a BUG. If you hit the end of input, then your code will go into an infinite loop. Mainly because fail() does not detect the end of input and operator>> will not update input correctly either. So not quite as bullet proof as you were hoping.

Also the use of peek() here seems a bit brittle. If the user typed in a valid input but stuck a space after it before the new line then your code would reject that users input. That's a pretty hard corner case to explain to a user especially if your error message is not very helpful.

In this chunk of code:

{
 cin.clear();
 cin.ignore(numeric_limits<streamsize>::max(), '\n');
 cout << message;
}
cin.clear();
cin.ignore(numeric_limits<streamsize>::max(), '\n');

You seem to have the same bit of code repeated twice. That's never a good sign.