Overall:
Good idea.
But your checks seem to be designed for machine input (not user input where a user could be told about the error and can then retry). But on the other hand you are prompting for specific details which is for humans and would only slow the machine reader down. If I assume your primary use case is for "manual human input" then retry logic being built into your class is much better than throwing an exception (which is also why streams don't throw exceptions by default).
I liked the idea for validation of the input but would like the ability to chain validators so that I don't have to create a specific validator for a scenario (but could rather chain existing validators).
I don't like that you are limiting things to specific category types, I would use the language's ability to use "Duck" typeing to define how the validators work for those specific types.
Code Review:
This class seems valid as it has more state than a simple runtime error
StreamFailure(const std::ios::iostate state)
: std::runtime_error{ "Stream failed (non-recoverable state)" },
m_state{ state }
But the following classes may be superfluous. But it depends. Do you see users explicitly catching these errors. If the answer is yes then go ahead and keep them. But if they are simply there for the static strings they contain I am not sure they are worth the effort.
class ExtractionFailure final : public std::runtime_error
class PredicateFailure final : public std::runtime_error
class BadPredicateParams final : public std::runtime_error
Personally. I create exception classes specifically because I think that it is worth catching the exceptions. It is worth catching the exception when there is potential corrective actions available for the user of my interface. Thus in the exception I document what it means and what I think the catcher "can" do to compensate as part of the exception.
If the exception is just a generic warning I use one of the standard exceptions and dump some information to a log file.
A lot of C++ programmer are used to use iterator like ranges. So end
is one past the end. In the class Between
you are not using it this way. Here end is within the range. Mathematically you are probably correct, but you have to consider your audience.
class Between final
{
public:
...
constexpr bool operator()(const T value) const noexcept
{
return value >= m_min && value <= m_max;
}
...
};
Why are you limiting to arithmetic types?
static_assert(std::is_arithmetic<T>::value,
"Only arithmetic types are supported");
A lot of code in C++ uses duck typing. If the type defines the appropriate operators then why should you not be able to use it with these classes.
Same comment here:
class MinMaxSymbols final
static_assert(std::is_same<std::string, T>::value,
"Only suported for std::string");
If the class T
supports the size()
method then why not allow the use of this class to validate it.
If you only want to support string. Why are you limiting to char
type strings. Why is their no support for wchar_t
strings?
Design issue on input()
functions.
- You are forcing an extra move operation (which on some types may be a copy).
- You only support
std::cin
?
I suppose if this is only for direct manual user input then that is fine. But a lot of time the program will be developed using user input to test and validate but when it goes into production you will then be streaming input from a file or socket (or the output of one command will be streamed to the input of this command).
I would make Input
a wrapper class then overload the operator>>
.
So you usage would look like this:
int i = input<int>("Testing: ");
I would change it to look like:
int i;
// Allows any input stream.
// Streams directly into the variable.
// If you are not using "human input" then turn of re-checking
std::cin >> Input(i, "Testing: ", humanInput);
--
// You are forcing a zero construction of `temp`
T temp{};
// But now you overwrite the default construction.
// Seems like the initial forced zeroing is wasted.
std::cin >> temp;
// The stream class already has a way to automatically throw
// when things go wrong.
// std::cin.exceptions(std::ios::eofbit | std::ios::badbit);
// see: https://cplusplus.com/reference/ios/ios/exceptions/
if(std::cin.eof() || std::cin.bad())
{
throw StreamFailure{ std::cin.rdstate() };
}
constexpr auto max{ std::numeric_limits<std::streamsize>::max() };
if(std::cin.fail())
{
std::cin.clear();
std::cin.ignore(max, '\n');
throw ExtractionFailure{};
}
std::cin.ignore(max, '\n');
return temp;
}
If this is truly checking interactive user input. Then should you not generate an error message and ask the user to try again. The only time worth bailing is if this is NOT interactive user input (it can't be fixed so stop processing now).
At the moment, you are forcing the user of your code to wrap this in a try catch block.
int result;
while (true) {
try {
result = input<int>("Testing: ");
break;
}
catch(ExtractionFailure const& e) {
std::cout << "Bad Input: Try again\n";
}
}
I would do soemmthing like this:
template<typename T>
class Input
{
T& dst;
std::string prompt;
bool human;
constexpr std::sszie_t max{ std::numeric_limits<std::streamsize>::max() };
public:
Input(T& d, std::string prompt, bool human)
: dst(d)
, std::move(prompt)
, human(human)
{}
friend std::istream& operator>>(std::istream& s, Input& dest) {
dest.read(s);
return s;
}
void read(std::istream& s)
{
while (true)
{
if(!prompt.empty() && human)
{
std::cout << prompt << std::endl;
}
if (s >> temp) {
// It worked
// If this is human "Line" based input then anything
// else on the line may be an error. If this is
// machine readable input then it may not be line based.
break;
}
if(s.eof() || s.bad())
{
// Non-correctable error
throw StreamFailure{ std::cin.rdstate() };
}
// Correctable Error.
// So reset the stream if this is a human.
if (!human) {
throw ExtractionFailure{};
}
// Human input line based.
// So clear error ignore to the end of the line
s.clear();
s.ignore(max, '\n');
std::cout << "Bad input: Try again\n";
}
}
};
This alternative allows a single predicate.
template<typename T, template<typename> class Predicate>
T input(const std::string& prompt, Predicate<T> pred)
{
T temp{ input<T>(prompt) };
if(!pred(temp))
{
throw PredicateFailure{};
}
return temp;
}
Have you though about chaining tests so that you can apply multiple?
MyObject test;
std::cin >> Input(test, "Add MyObject input: ", human) >> ValidateNerve(true) >> ValidateBundled(true) >> CheckRange(5,6);