Easy integer input

Question

I'm writing this app that's based in the terminal, and of course there's a lot of input involved, especially numerical input. I've decided to use the method of reading an entire line of input and converting into a number and displaying an error message if the input is out of bounds or just straight-up invalid. This happens inside and infinite loop so that the user can keep trying until they get it right.

Having to do this so many times in my code has resulted in me having to move it to function in a namespace like so:

io_helpers.h

#ifndef IO_HELPERS_H
#define IO_HELPERS_H
#include <string>
namespace easyio
{
 bool fileExists(const std::string& filename);
 int readInt(const std::string& prompt, int min, int max, const std::string& errorMsg);
}
#endif

io_helpers.cpp

#include "io_helpers.h"
#include <fstream>
#include <iostream>
using namespace std;
bool easyio::fileExists(const string& filename)
{
 ifstream fin(filename);
 if (fin.is_open())
 {
 return true;
 }
 else
 {
 return false;
 }
}
int easyio::readInt(const string& prompt, int min, int max, const string& errorMsg = "invalid input!")
{
 while (true)
 {
 try
 {
 string input;
 cout << prompt << ": ";
 getline(cin >> ws, input);
 int number = stoi(input);
 if (number < min || number > max)
 throw exception();
 return number;
 }
 catch (exception& e)
 {
 cout << errorMsg << endl;
 }
 }
}

Is this method of input the best method though? Is there anything I can do to improve the user experience or the performance? And on a side-note, should this be a static method in a class or a function in a namespace, or does it not matter?

• barak manos: Why did you chose to put function fileExists outside the namespace? In addition, you might as well pass const std::string& (like you do in the other function). Third, you're not returning anything in the catch clause. Perhaps you want to throw the exception to the caller function? To me it looks like there's no need to "get smart around" with those exceptions there. Simply pass int errorVal to the function, and return it in case you scan an input value out of range.
• rcplusplus: Oh yeah, it would be better to put fileExists in the namespace, as would passing a const std::string&... chalk that up to absent-mindedness. As for returning from the catch clause, I don't need to, because it will just loop back into the try clause if an exception is thrown (unless I'm wrong in how exceptions work, which I could be; I don't use them often).
• barak manos: No, you're right. I've missed the while (true).

Answer 1

Don't use an if statement when both branches do the same thing.

if (fin.is_open())
{
 return true;
}
else
{
 return false;
}

Easier to just return.

return fin.is_open();

Using exceptions for control flow is not a good idea:

 if (number < min || number > max)
 throw exception();

Exceptions are good for moving control from the current location to a location you can not know about at the time of development. This is usually because you can not fix the problem locally.

In this situation you can fix the problem locally and thus error codes are better (as long as they do not cross any public API).

 try {
 // Get input:
 if (number >= min && number <= max) {
 return number;
 }
 }
 catch (std::exception const&) { // catch by const reference
 // Ignore exceptions.
 }
 std::cout << errorMsg << "\n";

Personally I don't see the point in dropping white space here: cin >> ws as all other routines are going to drop white space including stoi(). It just makes the code harder to read.

You also don't check for garbage on the end of the line. What happens if I type 5x5<enter> does that have meaning? Persoanlly I would do some more checking:

std::string line;
std::getline(std::cin, line);
std::stringstream linestream(line);
int value;
char x;
if ((linestream >> value) && (!linestream >> x))
{
 // We only enter here iff
 // 1) We successfully read an integer from the stream.
 // 2) We failed to read a following (non white space) character.
 // This means there was only a number on the line.
 return value;
}

Usual warnings about this:

using namespace std;

Best practive is to always use {} on sub statements:

 if (number < min || number > max)
 throw exception();

I would have written:

 if (number < min || number > max) {
 throw exception();
 }

• rcplusplus: really valuable improvements and overall tips, thank you! Man, I can't I was using if to test a boolean, my face is quite red...
• Loki Astari: @BenVoigt: Got up on the wrong side of the bed this morning?
• Loki Astari: @BenVoigt: You must have a really bad hangover or something.