I like the idea; in fact, this is something I’ve considered doing myself from time to time.
Before I get into the review, I have to ask: why is this tagged C++14? It seems peculiar to tie yourself to a standard that is almost a decade old. C++23 is likely only a couple weeks away. These days, I would expect, at a minimum, C++17 support… if not C++20.
Anyhow, on to the review:
The very first thing I notice is that you have no namespace. All code should be namespaced; you should never put code in the global namespace. It’s good practice to have your own personal namespace—say namespace globalturist—and then nested namespaces for all your projects, libraries, etc..
class StreamFailure final : public std::runtime_error
There is already a standard exception for IOstreams failures, and streams can automatically throw it if you choose.
std::ios_base::failure doesn’t truck around a copy of the stream state flags… but then, it doesn’t need to. If anyone wants to see what the state of std::cin is, they can just ask it directly.
I would ditch both StreamFailure and ExtractionFailure, and just use the standard exception.
I would go even further, and not throw exceptions on stream failures, but more on that later.
class PredicateFailure final : public std::runtime_error
I’m not sure what the benefit is to marking this exception as final. Indeed, I don’t see the point in marking every class final; that just seems seems pointlessly gratuitous, and offers no real benefits. In particular here, though, there are a lot of very good reasons why someone might want to create a more derived exception. Just consider your Between predicate. I might want more information, so I can give better error messages, so:
class value_out_of_range : public PredicateFailure
{
public:
value_out_of_range()
: value_out_of_range{"value is out of range"}
{}
explicit value_out_of_range(std::string const& msg)
: PredicateFailure{msg}
{}
};
class value_too_large : public value_out_of_range
{
public:
value_too_large()
: value_too_large{"value too large"}
{}
explicit value_too_large(std::string const& msg)
: value_out_of_range{msg}
{}
};
class value_too_small : public value_out_of_range
{
public:
value_too_small()
: value_too_small{"value too small"}
{}
explicit value_too_small(std::string const& msg)
: value_out_of_range{msg}
{}
};
// Using C++17 NTTPs; can do the same in C++14, but more verbose
template <auto Min, decltype(Min) Max>
class between
{
public:
static_assert(Min < Max);
constexpr auto operator()(T const& v) const
{
if (v < Min)
throw value_too_small{};
if (Max < v)
throw value_too_large{};
return true;
}
};
try
{
auto i = input<int>("input dice value: ", between<1, 6>{});
}
catch (value_too_small const&)
{
std::cerr << "value was too small";
}
catch (value_too_large const&)
{
std::cerr << "value was too large";
}
Generally, making a more specific and precise error than just “predicate failure” seems useful. I don’t see the logic in forbidding it.
class BadPredicateParams final : public std::runtime_error
If someone passes bad arguments to a predicate, that would seem to be a logic error, not a run-time error. A logic error is one that can be detected and fixed before actually attempting the operation. For example, “file open error” is a run-time error, because there is no way to know the file won’t open until you actually try to open it. “Invalid argument” is a logic error, because you can check the argument before you pass it to the function, to see if it’s valid.
And, incidentally, not only should this class probably inherit from std::logic_error, there is already std::invalid_argument. You could either replace this class with std::invalid_argument, or derive from it.
On to the Between predicate….
static_assert(std::is_arithmetic<T>::value,
"Only arithmetic types are supported");
But why?
Throughout your code, there is a pattern of over-constraining. You make all classes final, you make all function parameters const, and you constrain all your predicates unnecessarily. I would advise that you chill a bit; it is not necessary to be so restrictive. You should not constrain stuff unless there is a good reason to. Otherwise, leave things open, so other code can adapt, extend, or otherwise build on what you’ve provided.
In this case, not only is there no sensible reason to restrict this predicate class to arithmetic types, this constraint actually manages to both over-constrain and under-constrain at the same time. You prevent, for example, using strong alias types, extended integer types, non-numeric types that have a concept of “between”, and so on… but at the same time, Between<bool> is legal.
Generally, trying to constrain types like this is a sucker’s game in C++17 and before. Until concepts came along in C++20, seriously constraining stuff was a massive headache, when it was possible at all. What you actually want to do here, syntactically, is constrain to types that support comparisons. Traditionally, in C++, that means the less-than operator. So you want to constrain to types that support less-than comparisons that return bool. You could write that constraint in C++17 and before… but it would not be trivial. I wouldn’t bother. Either move to C++20 and use a concept, or forget about it.
And that’s just the syntactic constraints. There are also semantic constraints, and they can get extremely complex. Even here, things are much more complicated than you think. You want to restrict the Between predicate to types that are ordered… that is, types that support operator<. Except, there are different kinds of ordering! Do you want to restrict to totally ordered types? Probably not. You probably only want weak ordering. Strict weak ordering? Maybe, maybe not. As you can see, there are many flavours of “ordered”.
My advice:
- don’t constrain stuff unless you are sure you know:
- what the constraints actually are, and
- why they are the right constraints; and
- don’t bother to constrain anything without C++20 concepts, which not only provide much better syntactic constraint, but also provide semantics.
constexpr Between(const T min, const T max)
: m_min{ min }, m_max{ max }
Making the parameters const serves no purpose, and, in fact, inhibits the most basic optimization you should use when taking “sink” parameters:
constexpr Between(T min, T max)
: m_min{std::move(min)}
, m_max{std::move(max)}
It won’t make a difference for scalar types, of course, but there is no reason to restrict this predicate to scalar types. It would make a huge difference for more complex types, of course.
constexpr Between(const T min, const T max)
: m_min{ min }, m_max{ max }
{
if(min >= max)
{
throw BadPredicateParams{};
}
}
In C++, the fundamental comparison operations are equals (==) and less-than (<). You’ll notice that all algorithms and such are defined in terms of those operations (for example std::sort(), std::map, and so on). In theory, all other operations will be defined in terms of those. So, in practice, in generic code it makes sense to restrict comparisons to those operations. In this case:
constexpr Between(T min, T max)
: m_min{std::move(min)}
, m_max{std::move(max)}
{
if (not (m_min < m_max))
throw BadPredicateParams{};
}
The same idea applies to your greater-than predicate. Instead of:
return value > m_min;
you should do:
return m_min < value;
One other important thing I should mention: GreaterThen, SmallerThen, MinSymbols, MaxSymbols all have constructors that take a single value, often a simple integral type. None of them are marked explicit, which allows implicit conversions from those simple integral types. This is a bad thing. Single argument constructors should almost always be makred explicit.
Okay, now I note that you have GreaterThen, SmallerThen, and Between, and MinSymbols, MaxSymbols, and MinMaxSymbols. The list of predicates you are going to want is going to get enormous if you keep with this pattern.
I would suggest rethinking things. For example, you could define a basic predicate type that wraps any predicate, and support boolean operations to combine predicates. That way you could write:
// instead of:
auto name = input<std::string>(
"Enter a name (3-6 symbols): ",
MinMaxSymbols<std::string>{3, 6}
);
// you could do:
auto name = input<std::string>(
"Enter a name (3-6 symbols): ",
MinSymbols<std::string>{3} and MaxSymbols<std::string>{6}
);
// number must be greater than or equal 0.0, and
// less than (but not equal to!) 1.0.
auto number = input<double>("",
equal_to{0.0} or (greater_than{0.0} and less_than{1.0}))
That would greatly shrink the number of predicates you actually need.
Another option—and, I think, the best option—is simply to take arbitrary predicates:
auto number = input<double>("", [](double v) { return v >= 0.0 and v < 1.0; });
This effectively obsoletes all of your existing predicates. For example:
// this would still work:
auto v = input<int>("", GreaterThan<int>{5});
// but so would this:
auto v = input<int>("", std::bind(std::greater<>{}, _1, 5));
// or in C++23:
auto v = input<int>("", std::bind_back(std::greater<>{}, 5));
// or:
auto v = input<int>("", [](auto x) { return x > 5; });
// and if you really wanted a named, reusable type:
auto greater_than = [](auto n) {
return [n](auto x) { return x > n; };
};
auto v = input<int>("", greater_than(5));
Up to this point, I’ve basically said “throw this out” to everything. You don’t need most of your custom exceptions—there are standard exceptions that do the same thing—and you don’t need your predicates—you should just accept arbitrary predicates. But that stuff was all really ancillary stuff. Now we get to the actual meat of the library.
template<typename T>
T input(const std::string& prompt)
I’m torn about this interface. It’s simple, but it’s almost too simple.
The thing with exceptions is that they should be… exceptional. Which, of course, begs the question, what is exceptional? That’s always a tricky question, but particularly so when dealing with user input. How “exceptional” is it that a user would enter bad input? I would say… not very. By my thinking, you should only enter the exception handling system when things have gone unexpectedly and terribly wrong… and a user entering bad input is neither particularly rare nor catastrophic. Indeed, in a normal program run, it might be an honest and even common mistake.
On the other hand, there may be situations where it would be inconceivable and disastrous for a user to enter bad input.
But to handle the general case, I would consider using something like C++23’s std::expected. (You can make your own in C++14; it’s not complex.) That allows flexibility:
// assume:
template <typename T>
auto input(std::string const&) -> std::expected<T, std::exception_ptr>;
// you could do:
if (auto value = input<int>("give number: "); value)
use(*value);
else
std::rethrow_exception(value.error());
// or with the monadic interface:
input<int>("give number: ")
.and_then(use)
.or_else([](auto e) { std::rethrow_exception(e); })
;
// or if you think bad input will be rare, and want a clean happy path,
// you can use your existing pattern like so:
try
{
auto value = *input<int>("give number: ");
use(value);
}
catch (std::bad_expected_access<std::exception_ptr> const& x)
{
try
{
std::rethrow_exception(x.error());
}
catch (StreamFailure const& x)
{
std::cerr << x.what() << '\n';
std::cerr << "badbit: " << (x.state() & std::ios::badbit) << '\n';
std::cerr << "eofbit: " << (x.state() & std::ios::eofbit) << '\n';
std::cerr << "failbit: " << (x.state() & std::ios::failbit) << '\n';
}
catch(ExtractionFailure const& x)
{
std::cerr << x.what() << '\n';
}
catch (PredicateFailure const& x)
{
std::cerr << x.what() << '\n';
}
catch (std::exception const& x)
{
std::cerr << x.what() << '\n';
}
}
You could also have both options, by using a tag, so input<T>("") returns expected<T, exception_ptr>, and input<T>(throw_on_error, "") returns T, but throws on error.
The other thing that bothers me about the interface is that it is hard-coded to use std::cin/std::cout and nothing else. That’s inflexible, but more importantly, it means your code is functionally untestable. (Not literally untestable, because it is possible to mock std::cin and std::cout… it’s just hard.) If your code is untestable, it will not be tested. Untested code is garbage code, not in the sense that it is “bad”, but in the sense that it is utterly useless in any real world project that expects rigorously tested code… which is any serious project… so it’s effectively garbage.
I would suggest taking a step back and rethinking the public API. It should be easy to use, flexible, and also testable. So, maybe:
template <typename T, typename Predicate>
auto input(std::istream& in, std::ostream& out, std::string_view prompt, Predicate&& pred)
-> std::expected<T, std::exception_ptr>;
template <typename T, typename Predicate>
auto input(std::string_view prompt, Predicate&& pred)
{
return input<T>(std::cin, std::cout, prompt, std::forward<Predicate>(pred));
}
template <typename T>
auto input(std::string_view prompt)
{
return input<T>(std::cin, std::cout, prompt, [](auto&&) { return true; });
}
For even more flexibility, you could even do:
template <typename T, typename Char, typename Traits, typename Predicate>
auto input(
std::basic_istream<Char, Traits>& in,
std::basic_ostream<Char, Traits>& out,
std::basic_string_view<Char, Traits> prompt,
Predicate&& pred
)
-> std::expected<T, std::exception_ptr>;
template <typename T, typename Predicate>
auto input(std::string_view prompt, Predicate&& pred)
{
return input<T>(std::cin, std::cout, prompt, std::forward<Predicate>(pred));
}
template <typename T, typename Predicate>
auto input(std::wstring_view prompt, Predicate&& pred)
{
return input<T>(std::wcin, std::wcout, prompt, std::forward<Predicate>(pred));
}
template <typename T>
auto input(std::string_view prompt)
{
return input<T>(std::cin, std::cout, prompt, [](auto&&) { return true; });
}
template <typename T>
auto input(std::wstring_view prompt)
{
return input<T>(std::wcin, std::wcout, prompt, [](auto&&) { return true; });
}
And, of course, you could add overloads that take a tag, and return T instead of std::expected<T, std::exception_ptr>, but throw on error.
You’ll note I used C++17’s std::string_view rather than std::string const& parameters, because that is the proper way to take string values you just want to inspect. If you’re stuck with C++14, you’ll either have to roll your own, use a library, or just bite the bullet and accept you will be paying the cost of string constructions unnecessarily.
Now let’s actually dive into the function:
T temp{};
std::cin >> temp;
It’s important to note that you are relying on T being default constructible (and cheaply so). That won’t always be true, so to support other types you should probably add some overloads that take an initial T value somehow, and use that to input into. Avoid copying it, too, because that would just be assuming it is copyable. This can get really tricky!
T temp{};
std::cin >> temp;
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');
Okay, you’re doing IOstreams error handling all wrong. For starters, EOF is not an error. Let me illustrate with this simple program:
# buildfile ####################################################
using cxx
exe{prog} : cxx{*}
// prog.cxx ////////////////////////////////////////////////////
#include <iostream>
auto main() -> int
{
using type = int;
auto v = type{};
std::cin >> v;
std::cout.setf(std::ios_base::boolalpha);
if (std::cin)
std::cout << "SUCCESS! ";
else
std::cout << "FAILURE! ";
std::cout << "v = " << v << '\n';
std::cout << "okay: " << bool(std::cin) << '\n';
std::cout << "good: " << std::cin.good() << '\n';
std::cout << "eof: " << std::cin.eof() << '\n';
std::cout << "fail: " << std::cin.fail() << '\n';
std::cout << "bad: " << std::cin.bad() << '\n';
}
$ b
c++ cxx{prog} -> obje{prog}
ld exe{prog}
$ printf '123\n' | ./prog
SUCCESS! v = 123
okay: true
good: true
eof: false
fail: false
bad: false
$ printf '123' | ./prog
SUCCESS! v = 123
okay: true
good: false
eof: true
fail: false
bad: false
$
You can see that we got successful reads both when EOF was set and when it was not. Here are two more examples to show how things work:
$ printf '123x456' | ./prog
SUCCESS! v = 123
okay: true
good: true
eof: false
fail: false
bad: false
$ printf 'x123x456' | ./prog
FAILURE! v = 0
okay: false
good: false
eof: false
fail: true
bad: false
$
So EOF is not an error, but fail always is. fail will always be true on an input failure, regardless of the cause. It will never be true if anything was successfully read, regardless of the state of the stream. In fact, operator bool for streams is literally just not fail().
So the correct way to check for valid input is either not cin.fail(), or just bool(cin). In other words:
T temp{};
if (not (std::cin >> temp))
{
// there was some kind of error
// If std::cin.bad() is true, then cin is broken. There is
// nothing you can do. It's over.
// If std::cin.eof() is true, then there is no more input.
// Again, there is nothing you can do. It's over.
// If neither of the above is true, then there was just a
// parsing error. There may be more input, and it may even
// be valid.
//
// If you are doing line-oriented input, as you seem to be,
// this would be where you'd ignore the rest of the line,
// but otherwise leave cin in a valid state. You'd return an
// error, because this input did fail, but you would allow
// further inputs.
}
// This is dangerous!!!
std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
return temp;
You’ll note I marked the ignore line as dangerous. That’s because, at that point, you have successfully read your input and are ready to return it… but if you then do the ignore and that fails… it may trigger an exception that wipes out all of your success so far. There is nothing you can do about that, though; if the user has enabled exceptions, this would be on their head. Still, it’s important to be aware that it might happen.
I would also point out that after a successful read, maybe you don’t want to just ignore the rest of the line. If I ask for a number, and the user does “123⏎” then that’s fine: I got “123” as input. But if the user does “123xyz⏎”… what does that mean? Should I accept “123” as input and ignore the rest? Seems dangerous, especially if the next input would have accepted “xyz”… maybe the user was just typing too fast, and didn’t realize they’d made an error, which would create incorrect input for the rest of the program… which might be very dangerous. Imagine:
enter the code number: 123
should i delete the entire hard drive: n
begin operation? y
done; quit now? y
That’s “123⏎n⏎y⏎y⏎”. But the user doesn’t hit the enter key hard enough after the first line, so it’s “123n⏎y⏎y⏎”, and “123n” gets read as “123”, the “n” gets ignored, and the next input… which is “y”… goes to the next line. You end up one “y” short… for the question about quitting, after the hard drive has already been erased. Ouch.
On top of that, reading input from std::cin directly is generally a dodgy prospect. You don’t know what the user has done with std::cin. Have they enabled exceptions? Have they changed the locale (which would affect how numbers are read)?
I would advise a different pattern. You are reading line-based input. I would suggest leaning into that, and actually reading whole lines from std::cin, and parsing them after the fact. For example:
template <typename T>
auto input(std::string const& prompt)
{
// What happens if this output fails? Maybe you should check
// for that, and throw?
if (not prompt.empty())
std::cout << prompt;
auto line = std::string{};
if (not std::getline(std::cin, line))
// If this failed, then either cin was bad, or there was
// no more input to read. So:
throw StreamFailure{std::cin.rdstate()};
// If T is std::string, then we’re done! Just return `line`!
// Now we create an istringstream with line as the source data:
auto iss = std::istringstream{std::move(line)};
// Optional, copy the formatting data and locale from std::cin:
iss.copyfmt(std::cin);
iss.exceptions(std::cin::goodbit); // we don't want exceptions
//
// ... or do we? you could
// enable exceptions, and they
// they will be automatically
// thrown... but it will be
// `std::ios_basefailure`,
// not your custom exception.
auto temp = T{};
if (not (iss >> temp))
// No need to do any cleanup on cin!
throw ExtractionFailure{};
// Okay, now we have successfully read a T value... but was
// there any trailing garbage? Easy to test!
if (iss.peek() != std::istringstream::traits_type::eof())
// There is trailing garbage.
throw ???;
return temp;
}
You can see how much easier that is, because we only touch std::cin once. If there are any errors with the stream itself, we detect them on that one getline() read. But once that succeeds, the only errors will be parsing errors, and if that happens… well, std::cin is already in the correct state, so we don’t need to mess with it again.
And there are a lot of other benefits from doing it this way. For example, we don’t need to construct T at all if std::cin is bad.
Alright, moving on:
template<>
std::string input(const std::string& prompt)
Don’t specialize function templates.
What you want to do is delegate to a class, and specialize that, if necessary:
template <typename T>
struct input_impl
{
auto operator()(std::string const&) -> T
{
// ...
}
};
template <>
struct input_impl<std::string>
{
auto operator()(std::string const&) -> std::string
{
// ...
}
};
template <typename T>
auto input(std::string const& prompt)
{
return input_impl<T>{}(prompt);
}
Or, even better, use the modern “neibloid” pattern:
template <typename T>
struct input_impl
{
auto operator()(std::string_view prompt) const
-> std::expected<T, std::exception_ptr>
{
return input<std::string>(prompt)
.and_then([](auto line)
{
try
{
auto iss = std::istringstream{std::move(line)};
iss.copyfmt(std::cin);
iss.exceptions(std::cin::goodbit);
auto temp = T{};
if (not (iss >> temp))
throw std::ios_base::failure{"failed to parse value"};
if (iss.peek() != std::istringstream::traits_type::eof())
throw std::ios_base::failure{"trailing garbage after parsed value"};
return temp;
}
catch (...)
{
return std::unexpected{std::current_exception()};
}
});
}
};
template <>
struct input_impl<std::string>
{
auto operator()(std::string_view prompt) const
-> std::expected<std::string, std::exception_ptr>
{
try
{
if (not prompt.empty())
std::cout << prompt;
auto line = std::string{};
if (not std::getline(std::cin, line))
throw std::ios_base::failure{"could not read from cin"};
return line;
}
catch (...)
{
return std::unexpected{std::current_exception()};
}
}
};
template <typename T>
inline constexpr auto input = input_impl<T>{};
Moving on:
template<typename T, template<typename> class Predicate>
Okay, you’ve already run into problems with Predicate being a template template parameter:
Also, I cant remove the templates from the string predicates MinMaxSymbols, MinSymbols, MaxSymbols because the input function that accepts the predicate is a template template.
So… why? Why is the predicate a template template parameter? What does that help?
The thinking seems to be that for any type T<U> that has a function call operator, that function call operator will take a U. In other words:
template <typenaem U>
struct T
{
auto operator()(U) const -> bool;
};
But that’s not really true. There’s no reason a template that takes a single argument would have an operator call at all (example: std::atomic<T>). And there are a lot of cases where a template that takes a single parameter has a call operator that takes a different type (example: std::uniform_int_distribution<T>).
So there is really no sensible reason to constrain the predicate only to types that are:
- templates
- with only one template parameter
- which is the same type as the call operator parameter.
And even with all that over-constraining, the predicate is still under-constrained, because you don’t check that it returns a type convertible to bool.
Again, don’t bother constraining until C++20. It’s not worth it. In C++20, constraining the predicate is simple:
template <typename T, std::predicate<T> Predicate>
auto input(std::string_view prompt, Predicate pred) -> T;
That’s pretty much all you need.
Replicating this behaviour before C++20 is very hard, though. It’s just not worth it.
If you just make Predicate an ordinary template parameter, you no longer need the superfluous std::string template arguments for all those predicates, and you allow arbitrary predicates. There is no sensible reason to constrain the predicate to be a template, let alone a template that specifically only takes a single parameter, that happens to be the same type that you’re testing.
I’m not going to bother reviewing the main() function, because it’s just for testing/demonstration purposes. I will point out that if you made the interface properly testable, you could write much better tests that don’t require manual checking. For example:
// Not using any particular testing platform. Also, this test
// should actually be split into multiple tests - one for the
// value, one to make sure the prompt works. This is just a
// very basic demo.
TEST
{
auto oss = std::ostringstream{};
auto iss = std::istringstream{"1234"};
auto value = input<int>(iss, oss, "gimme int: ");
CHECK(value == 1234);
CHECK(oss.str() == "gimme int: ");
}
Before I wrap up the review, I’ll address some of your concerns:
The custom exceptions are mostly glorified std::runtime_error.
That’s not really a bad thing. It’s just unfortunate that deriving exceptions requires so much boilerplate.
Personally, I like to use Boost.Exception. It massively simplifies making exceptions, and gives a lot of extra power when using them. Most of my projects have error handling structured like this:
namespace indi {
struct exception
: virtual std::exception
, virtual boost::exception
{};
namespace inputlib {
struct predicate_failure : virtual exception {};
struct input_value_out_of_range : virtual predicate_failure {};
struct input_value_too_large : virtual input_value_out_of_range {};
struct input_value_too_small : virtual input_value_out_of_range {};
struct input_value_length_error : virtual predicate_failure {};
struct input_value_too_long : virtual input_value_length_error {};
struct input_value_too_short : virtual input_value_length_error {};
} // namespace inputlib
} // namespace indi
auto func()
{
// in cases where i want to handle specific error types:
try
{
// ...
}
catch (indi::inputlib::input_value_too_long const&)
{
std::cerr << "input too long!";
}
catch (indi::inputlib::input_value_too_short const&)
{
std::cerr << "input too short!";
}
}
// in main() to print diagnostics for any unhandled exceptions
// (technically i usually use a lippincott function, but meh)
auto main() -> int
{
try
{
// ...
}
catch (...)
{
std::cerr
<< "[ERROR]:\n"
<< boost::current_exception_diagnostic_information();
}
}
I would mention that std::runtime_error is not always the right error to use. For logic errors, you should use a type that derives from std::logic_error, not std::runtime_error. And there are several pre-defined exception types that are more suitable for specific errors.
The Arithmetic and String predicates repeat the same static_asserts. However, what bothers me more is that MinMaxSymbols could inherit from MinSymbol and MaxSymbol, but from what I read [isocpp][2] STRONGLY recommends you dont use multiple inheritance for code reuse
I mean, it’s not really a bad thing that the static asserts are repeated. If you really wanted, you could make a base class with all the required static asserts, and derive from that. But I wouldn’t bother.
Yes, making MinMaxSymbols multiply-derive from MinSymbols and MaxSymbols would be a terrible idea, for a number of reasons. First of all, it would require turning a simple call into a virtual call, for no good reason.
More importantly, though, it would be nonsensical, because a MinMaxSymbols predicate is not a MinSymbols predicate or a MaxSymbols predicate, let alone both. Suppose I had a MinSymbols{3}, and you were relying on its behaviour, which is that it will pass "abc" and "abcdefg", but fail "ab". Then I silently replace it with MinMaxSymbols{3,6}… which should work if a MinMaxSymbols{3,6} IS-A MinSymbols{3}. Except now, the behaviour you were relying on is no longer true. "abc" still passes, but now "abcdefg" does not. So, clearly, MinMaxSymbols{3,6} is not a MinSymbols{3}, and should not derive from it.
Inheritance is the wrong model here. What you want is composition:
struct MinSymbols
{
// ... what you already have ...
};
struct MaxSymbols
{
// ... what you already have ...
};
class MinMaxSymbols
{
MinSymbols _min_check;
MaxSymbols _max_check;
public:
constexpr MinMaxSymbols(std::size_t min, std::size_t max)
: _min_check{min}
, _max_check{max}
{
if (max < min)
throw BadPredicateParams{};
}
constexpr auto operator()(std::string const& s) const noexcept
{
return _min_check(s) and _max_check(s);
}
};
I would even use composition for the GreaterThen and SmallerThen, using the standard function objects. For example:
template <typename T, typename Compare>
class comparison_operation
{
public:
constexpr explicit comparison_operation(T limit) noexcept
: _limit{std::move(limit)}
{}
constexpr auto operator()(T const& value) const noexcept
{
return _compare(value, _limit);
}
private:
[[no_unique_address]] Compare _compare;
T _limit;
};
template <typename T>
using greater_than = comparison_operation<T, std::greater<T>>;
template <typename T>
using greater_than_or_equal = comparison_operation<T, std::greater_equal<T>>;
template <typename T>
using less_than = comparison_operation<T, std::less<T>>;
template <typename T>
using less_than_or_equal = comparison_operation<T, std::less_equal<T>>;
Apart from that T input(const std::string& prompt) and std::string input(const std::string& prompt) repeat almost the same code but I dont see an elegant way to avoid that.
Sometimes, the best thing to do is take a step back, go do something else, and come back to look at a problem with a fresh eye.
I made a lot of suggestions for refactoring, but let’s ignore them, and look at your code as it is.
So, your string input function is this:
template<>
std::string input(const std::string& prompt)
{
if(!prompt.empty())
{
std::cout << prompt;
}
std::string temp{};
std::getline(std::cin, temp);
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{};
}
return temp;
}
Now, there’s nothing interesting about the first part (outputting the prompt). The next part… well, I mean, it’s only three statements, two of which are identical to the general input function, but one differs; when only 2 very simple lines of code are duplicated (and separated by a line that differs), it’s probably not worth worrying about duplication.
But then there’s the third part, which handles the case where parsing the string fails.
…
…
Did you catch that?
Let me repeat it: the third part handles the case where parsing the string fails.
…
See it now?
If not, just consider these questions: What, exactly, are you parsing when just directly inputting a string? And… how can that fail?
Here’s another way of looking at it. Suppose your input is “the first line⏎the second line⏎the third line⏎”. Suppose reading the first line fails—don’t ask me how, just assume for the thought experiment. So, the getline() call would read “the first line⏎”, leaving “the second line⏎the third line⏎” in the input. But there was a failure, so you enter that if block, where the fail bit is cleared, and then… you… ignore… everything… up to the next newline. So… “the second line⏎” just gets ignored… and the input is now “the third line⏎”. You just lost a whole line of input.
😬
So, here’s where I just give the punch line:
- string input “parsing” can never fail, because string input is not parsed, it’s just read; and
- even if it could fail, you have already consumed the whole line.
Which means… there’s actually a lot less duplicated logic here than you thought.
Here is what your string input function should look like:
template<>
std::string input(const std::string& prompt)
{
if(!prompt.empty())
{
std::cout << prompt;
}
std::string temp{};
std::getline(std::cin, temp);
if(std::cin.eof() || std::cin.bad())
{
throw StreamFailure{ std::cin.rdstate() };
}
// Parsing cannot fail, so this whole block is superfluous.
//
// But even if it weren't, you should not do the ignore().
/*
constexpr auto max{ std::numeric_limits<std::streamsize>::max() };
if(std::cin.fail())
{
std::cin.clear();
// std::cin.ignore(max, '\n'); // <-- DON'T DO THIS!!!
throw ExtractionFailure{};
}
*/
return temp;
}
Or, in other words:
template<typename T>
T input(const std::string& prompt)
{
if(!prompt.empty())
{
std::cout << prompt;
}
T temp{};
std::cin >> temp;
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;
}
template<>
std::string input(const std::string& prompt)
{
if(!prompt.empty())
{
std::cout << prompt;
}
std::string temp{};
std::getline(std::cin, temp);
if(std::cin.eof() || std::cin.bad())
{
throw StreamFailure{ std::cin.rdstate() };
}
return temp;
}
A lot less duplication than you thought, eh?
But, just for fun, let’s pretend that the two functions are identical, except for that one line. Could we avoid the duplication? Trivially! You’re just specializing in the wrong place:
template <typename T>
auto do_input(T& t)
{
std::cin >> t;
}
auto do_input(std::string& s)
{
std::getline(std::cin, s);
}
template <typename T>
auto input(std::string const& prompt)
{
if(!prompt.empty())
{
std::cout << prompt;
}
T temp{};
do_input(temp);
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;
}
See? Sometimes stepping back and coming back to code with a fresh eye can be magical!
Summary
The code is largely quite good. I didn’t detect any real bugs, except for the issues that arise from mishandling IOStreams exception handling. Aside from that, the code will work, just fine, in all cases, so far as I can tell.
It is also quite good stylistically and idiomatically; clear, logical, well-structured, except the one issue of specializing function templates.
Overall, quite good, bordering on excellent.
There are 3 main issues that come up repeatedly in the code.
There is a pattern of over-constraining.
Slapping final on every class is… absurd. I’m sure there is some crappy style guide out there that recommends this—there are a lot of crappy style guides out there (looking at you, Google style guide)—but it is generally pointless and actively crippling in some cases. final should only go on leaf classes in a hierarchy (which should usually be the first non-abstract class).
Slapping const on every function parameter seems less silly… but is just as absurd, though understanding why is trickier. Basically, there are multiple types of function parameters; if you are only reading their values, you should take them as const& (which makes const superfluous)… if you are taking their values—and giving them to data members, for example—then you should take them by value, and move them. The only time you should use const is for trivial types, which you will always take by value.
Insisting on types that conform to a very, very specific interface, when that interface is not actually required, is frustrating… as you discovered yourself when you had to make certain types templates for no good reason. If all you want is something with a call operator that takes a T and returns a bool… then that’s all you need. There is no point in demanding anything else. You don’t need it to be a template. You don’t need it to take T as its one and only template parameter.
Trying to constrain types before C++20 is a sucker’s game. It can be done, theoretically, in a limited way… but it takes so much effort, and requires so much complicated code using arcane tricks like SFINAE, and even then only offers limited payoff, that it’s really not worth it.
There is generally poor usage of the standard library.
There are a lot of cases where existing standard types serve the exact same purpose as your custom exceptions, like std::invalid_argument and BadPredicateParams and std::ios_base::failure and StreamFailure. That doesn’t necessarily mean you shouldn’t make your own custom exception types… but it does mean you should consider using those more specific library types as the base.
There are some misconceptions about error handling in the IOStreams library, which, to be fair, is quite complex.
The main type of failure you need to be concerned when reading input is “failure”. If you didn’t get a “failure” error, then you successfully read and parsed valid input. Doesn’t matter what else is going on in the universe; you got what you asked for.
Only after you detected a failure should you bother to discern whether it was a parse error without hitting the end of the stream (in which case, only failbit is set), a parse error because of hitting the end of the stream (in which case eofbit will be set), or a parse error due to a stream error (in which case badbit will be set). But always, always, always, the parse error comes first, so failbit will always be set on any kind of failure; if failbit is not set, then success, no matter what the other bits are.
In practice, that means all you need to do is if (in >> t) or if (std::getline(in, s)), and if that succeeds, you’re done. Both of those operations will internally construct the sentry and check for badbit and eofbit automatically. If that test fails, then it might have already thrown std::ios_base::failure or some other exception. If it didn’t throw, then, and only then, should you bother to check for badbit or eofbit yourself… and only if you want to throw an exception with extra information. But I wouldn’t bother; I’d just throw std::ios_base::failure. If the user cares about why it failed (parsing, EOF, or stream failure), they can always check the stream themselves. No point in doing that extra work if they don’t care, and if they do care, they need to do the work themselves anyway… so just leave it to them.
I also suggest a refactor to avoid template function specialization. At the very least, make a single function template and delegate to a class template (which can then be specialized as you please). But you might want to consider the neibloid pattern.
And I would suggest considering an interface that makes exceptions optional… just like IOStreams itself does. I can’t imagine most code wanting to input stuff won’t be expecting the possibility of bad input, so it’s hardly an exception situation. Returning a type similar to std::expected allows the user to decide whether they want to deal with malformed input exceptionally or not.
And finally, I would strongly suggest moving to a more modern standard. C++14 is ancient. All modern compilers default to C++17 these days. There is no good reason for new code to hamstring itself by using such an old standard.
