A week ago I posted this question and after reading the answers and accepting one I decided to refactor my code quite heavily. I would like to get some feedback on this newer version.
The changes
- No more predicates! I'd like it to look exactly like Python's
input()
function. - Always return a string and let the user parse it as needed (just like Python).
- Keeping 2 of the custom exceptions I made because I want the user to be able to handle them specifically. I know
std::ios_base::failure
exists but I want to separate it into different exception types.
The design choices I stuck with
- I'd like to keep
input()
as a function, so I'm not making it a class on purpose. - I know it is tightly coupled to
std::cin
and no other streams and it's on purpose. I'd like any new user to be able to just copy paste this input function and not worry about validation when reading input from the user. - I would like to keep this C++14 for now.
Validation rationale for std::cin
- cppref - Look at the code example
- learncpp - near the end of the lesson
Last thoughts
If I were to make this a class instead of a function I would just try to implement a Scanner (Java-like) that takes std::istream&
which would make it usable with any input stream. But for now I want to keep it std::cin
only because it will be shorter and simpler.
The code
#include <stdexcept>
#include <iostream>
#include <string>
#include <limits>
/* Custom exceptions -------------------------------------------------------- */
class StreamFailure final : public std::runtime_error
{
public:
StreamFailure()
: std::runtime_error{ "Stream failed (non-recoverable state)" }
{}
};
class ExtractionFailure final : public std::runtime_error
{
public:
ExtractionFailure()
: std::runtime_error{ "Failed to extract value from stream" }
{}
};
/* Simplified input --------------------------------------------------------- */
std::string input(const std::string& prompt = "")
{
if(!prompt.empty())
{
std::cout << prompt;
}
std::string temp{}; // <-- unavoidable if i want it to look like python
std::getline(std::cin, temp);
if(std::cin.eof() || std::cin.bad())
{
throw StreamFailure{}; // <--should rarely happen for any type extracted
}
else if(std::cin.fail())
{
std::cin.clear();
std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
throw ExtractionFailure{}; // <-- pretty hard to make this happen with
// std::string
}
return temp;
}
int main()
{
while (true)
{
try
{
int i = std::stoi(input("Enter an integer: "));
std::cout << i << '\n';
}
catch (std::invalid_argument const& e)
{
std::cerr << "invalid_argument\n";
}
catch (std::out_of_range const& e)
{
std::cerr << "out_of_range\n";
}
catch(const std::exception& e)
{
std::cerr << e.what() << '\n';
break;
}
}
return 0;
}
2 Answers 2
After reading @indi explain what problem I have with my code I've fixed it in accordance with the comments he left. If anyone sees any more problems with it you are welcome to point it out.
The new input function:
std::string input(const std::string& prompt = "")
{
if(!prompt.empty())
{
std::cout << prompt;
}
std::string temp{};
std::getline(std::cin, temp);
if(!std::cin)
{
if(std::cin.eof() || std::cin.bad())
{
throw StreamFailure{};
}
else
{
std::cin.clear();
std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
throw ExtractionFailure{};
}
}
return temp;
}
How about using std::error_code
instead of exception
?
From my experience using exception
for business logic will mess up the code.
#include <system_error>
#include <iostream>
enum struct MyError { //create own error value
//don't start with 0
invalid_input = 10 ,
out_of_range
};
namespace std
{
template <>
struct is_error_code_enum<MyError> : true_type {}; //overload template to accept our error value
}
struct MyCategory : std::error_category //create own category
{
const char* name() const noexcept override{
return "my_error";
}
std::string message(int value) const override{
switch(static_cast<MyError>(value)){
case MyError::invalid_input:
return "invalid input";
case MyError::out_of_range:
return "out of range";
default:
return "unknown error";
}
}
};
std::error_category& get_my_category() // singleton
{
static MyCategory cate{};
return cate;
}
std::error_code make_error_code(MyError value) //this overload function make_error_code must have namespace same with error value
{
return {static_cast<int>(value), get_my_category()};
}
///
std::error_code get_input(std::string& buffer, int dummy)
{
if(dummy == 1){// frist validate
return MyError::invalid_input; //implictly construct std::error_code from MyError::invalid_input. It potentially call make_error_code
}
if(dummy == 2){// second validate
return MyError::out_of_range;
}
//do valid thing
return {}; //return empty error
}
int main()
{
std::string buffer;
auto ec = get_input(buffer, 2);
if(ec //has error
&& ec != MyError::invalid_input // unhandle error
){
std::cerr << "[unhandle error] " << ec.message() << "\n";
return 0;
}
// handle error or use buffer
}
This seems better when you have a new error. Just add the new one to error value enum and message category.
If you don't like passing output as arguments you can use std::pair
to return both.
-
\$\begingroup\$ I doubt error codes would be better here. (1) there wont be any more new errors (2) With my code the exceptions should rarely even throw, the system should never really fail to read std::string unless you run out of memory or get a bad/eof stream which is highly unlikely meaning exceptions are exceptional (3) isocpp shows the problem with this approach. If I had to call the input function multiple lines in a row to read into different variables I would end up with an if/else if hell to make sure I handle all the errors \$\endgroup\$globalturist– globalturist2023年10月17日 06:52:57 +00:00Commented Oct 17, 2023 at 6:52
-
\$\begingroup\$ @globalturist std::error_code is associate with std::system_error. You can throw them with ` throw std::system_error{make_error_code(MyError::invalid_input)}`. \$\endgroup\$Jakkapong Rattananen– Jakkapong Rattananen2023年10月17日 10:28:03 +00:00Commented Oct 17, 2023 at 10:28
-
\$\begingroup\$ I know I can throw
std::error_code
, but please read the original question, I specified I leave in my custom exceptions on purpose. Also, Doesn't that beat your entire proposal of "How about using std::error_code instead of exception" ? \$\endgroup\$globalturist– globalturist2023年10月17日 10:49:15 +00:00Commented Oct 17, 2023 at 10:49
else
on line 19, then move lines 17 and 18 into theif
block (and indent them correctly to silence the warning/error). Now it works; it always accepts good input, always rejects bad input, and never loses good input. The lesson is simple and clear: "always check for "fail" first... then check for anything else (EOF or "bad"). If the fail bit was not set, then you have valid input, period, no matter what the EOF or bad bits are. \$\endgroup\$