Not a bad first attempt for a beginner, though you probably bit off more than you should have for a first attempt. As @JerryCoffin suggested, it would probably have been enough just to validate the input and return true/false. (However, even that is far, far more difficult than you’d think. Honestly, anything involving non-integer numbers is a fucking quagmire. I would not advise beginners to try any parsing or validation of any sort involving non-integer numbers.)
Before I get into reviewing your code, I’ll suggest how I would have approached the problem, because a lot of being a good C++ programmer has little to do with the C++ itself. Rather, it is learning to think like a software development engineer that really makes you a good C++ coder (and, as a bonus, those skills are easily transferable to other languages as well).
Okay, so the goal is to read a possibly negative decimal real number, and verify that it’s within a given range. Right away I imagine what a function that does that would have to look like. I would need to take the stream it’s trying to read the number from and the range, and it would need to return the number. So:
auto getVDecimal(std::istream& input_stream, value_range range) -> double;
where value_range is a type that holds a double range, so it probably looks something like this (to start):
struct value_range
{
double min;
double max;
};
I’ll get back to that type in a moment. (You’re probably wondering why I made a new type rather than just passing two doubles like you did. You’ll see in a moment.)
Now I ask… but what if there’s an error? How do I handle/report errors?
You’ve chosen to throw an exception, which is fine, and which means that the function signature above is good (because exceptions are thrown on a side channel, so you don’t need to change the function signature to support them).
(If instead the decision was not to use exceptions to signal errors, then I either would have used the stream’s error handling, or returned a std::expected<double, std:error_code>, or both… probably both.)
Now I look at the function signature again… I have to take the stream by non-const reference (that’s just required because of how streams work)… but the value range I only want to look at, for a read-only parameter the default is to use a const reference, so:
auto getVDecimal(std::istream& input_stream, value_range const& range) -> double;
This looks good to me, so I’ll use that as the function signature.
Now let’s take another look at value_range. The reason I created a specific type for this is because… that’s how you do C++. See, C++ is a strongly-typed language… it’s probably the most strongly-typed language you’ll ever use. To really get the max power out of C++, you need to use the type system. I always tell the people I teach: in C++, when you get the types right, everything else Just Works™. When you get the types right, everything else falls magically into place. I’ll demonstrate.
So value_range holds a pair of doubles… but not just any pair of doubles. It must be a pair where both doubles are valid numbers (no nans), and, it must be a pair where one double is less than or equal to the other. The latter is a very important point. Your function just takes two doubles… what’s to stop someone from calling it like this: getVDecimal(1.0, -1.0)? (Or this: getVDecimal(std::numeric_limits<double>::quiet_NaN(), std::numeric_limits<double>::quiet_NaN())?)
That means value_range needs to verify that the doubles it’s being given fit the bill:
class value_range
{
double _min = 0.0;
double _max = 0.0;
public:
constexpr value_range() noexcept = default;
constexpr value_range(double min_val, double max_val)
: _min{min_val}
, _max{max_val}
{
if (std::isnan(_min) or std::isnan(_max))
throw std::invalid_argument{"value_range bounds must be numbers"};
if (_min > _max)
throw std::invalid_argument{"value_range minimum bound is greater the maximum bound"};
}
constexpr auto min() const noexcept { return _min; }
constexpr auto max() const noexcept { return _max; }
// Handy helper function.
constexpr auto contains(double value) const noexcept
{
if (std::isnan(value))
return false;
return value >= _min and value <= _max;
}
};
With this type, the function is now ironclad safe… you literally cannot call it incorrectly. If you try to do getVDecimal(in_stream, {1.0, -1.0}) or getVDecimal(in_stream, {std::numeric_limits<double>::quiet_NaN(), std::numeric_limits<double>::quiet_NaN()}), you won’t get strange bugs… you’ll get an exception. (And it can be detected at compile time!)
And there’s more! With the contains() helper function, checking that a value is within the range no longer requires writing if (value >= minN && value >= maxN) out manually. And did you see what happened there? Oops, I made a typo and introduced a bug. But if (range.contains(value))… you can’t really introduce a typo bug with that. (Plus it reads a lot more nicely.)
That is what writing good C++ looks like: get the types right, and everything else Just Works™.
So, I have a function signature (and a helper type). What do I do next? …
…
…
I write the tests.
Yes, the function doesn’t exist yet. I’m writing the tests before I write the function. Because that’s how you do it.
And the tests are not hard. First you need to pick a testing framework. Let’s go with Catch2. So a simple test case would look like:
TEST_CASE("Simple test case")
{
auto input = std::istringstream{"-123.4"};
REQUIRE(getVDecimal(input, {-150.0, 150.0}) == -123.4);
}
And of course, you need to test expected error cases as well:
TEST_CASE("Value is too low")
{
auto input = std::istringstream{"-123.4"};
REQUIRE_THROWS_AS(getVDecimal(input, {-100.0, 150.0}), std::invalid_argument);
}
That’s the basic gist of it. Write as many tests as you please until you’re satisfied that you’ve covered all the edge cases.
(Incidentally, you should also write a whole set of tests just for value_range by itself, to make sure it works. You should test that initializing it with a valid range works, and the .min() and .max() member functions return the right values. You should test that .contains() works. And you should test that it throws when you try to create an invalid range.)
Once that’s done, I would have a whole wash of tests that won’t compile… because I haven’t written the function yet. So… I write this:
auto getVDecimal(std::istream& input_stream, value_range const& range) -> double
{
return 0.0;
}
Yup. That’s it.
With that, the tests will compile… but almost all of them will fail. That’s good. Now we can actually start coding the function.
This is where I’ll stop my example, because from here on out it’s just iterative improvements on the function. I add a bit of functionality, then I compile and run the tests again. Then I add a bit more, and run the tests again. Add more, run tests. Over and over, until all the tests pass.
And once all the tests pass, I’m done: I will know, with absolute confidence, that the function works.
(At that point I can start tweaking and optimizing the function, safe in the knowledge that if I accidentally break anything, the tests will tell me.)
That is how I would approach this problem:
- Consider the interface: What should the function signature look like to allow all the flexibility I want, yet still be safe to protect against accidental misuse?
- Write the tests.
- Write the code, checking the tests often, until all pass.
Now I’ll review your code.
Code review
void printErrorMsg(int err_code)
{
switch(err_code)
{
case -3:
std::cout << "- is allowed only at the beginning. Try again. ";
break;
case -2:
std::cout << "Too many separators! Try again. ";
break;
case -1:
std::cout << "Invalid input! Try Again. ";
break;
case 0:
std::cout << "Empty input! Try again. ";
break;
case 1:
std::cout << "No alphabets or punctuations allowed. Try again. ";
break;
case 2:
std::cout << "No separators allowed after decimal(.)! Try again. ";
break;
default:
break;
}
}
The major problem with this function is the fact that you’re using a naked int as an error code.
Remember, C++ is a strongly-typed language; it’s all about the types. That means you should create a custom type for every “thing” that is a distinct “thing” in your program. An error code is a very distinct thing. And an error code is definitely not an int. You can add ints; does it make sense to add error codes? Of course not, right?
For something like error codes, the best default thing to use is an enum. This is in fact the basis for std::error_code (though going the whole hog for std::error_code—making an error category and all—is a bit much for a beginner program, so we won’t go into all that).
So, what you should do is something more like this:
enum class decimal_input_errc
{
success = 0, // adding this as the first error enumerator is a good idea
hyphen_not_at_beginning,
too_many_separators,
invalid_input,
empty_input,
invalid_character,
separators_after_decimal_point
};
void printErrorMsg(decimal_input_errc err_code)
{
switch(err_code)
{
case decimal_input_errc::hyphen_not_at_beginning:
std::cout << "- is allowed only at the beginning. Try again. ";
break;
// ... and so on...
Even better would be to have a function specifically to convert the error code to a string, and keep that separate:
enum class decimal_input_errc
{
success = 0, // adding this as the first error enumerator is a good idea
hyphen_not_at_beginning,
too_many_separators,
invalid_input,
empty_input,
invalid_character,
separators_after_decimal_point
};
auto message(decimal_input_errc err_code) -> std::string
{
switch (err_code)
{
case decimal_input_errc::hyphen_not_at_beginning:
return "- is allowed only at the beginning.";
case decimal_input_errc::too_many_separators:
return "Too many separators!";
// ... and so on ...
default:
throw std::logic_error{"should never get here!"};
}
}
void printErrorMsg(decimal_input_errc err_code)
{
std::cout << message(err_code) << " Try again. ";
}
That way, if ever need to add/remove/change any of the error codes, you only need to do it in the error code enum itself, and the message() function. (It’s not perfect to have to keep changes in two places in sync, but it’s the best you can do without getting really advanced.)
void analyzeSeparator(const std::string *const input)
Taking arguments by pointer is… not a great idea. Not unless you intend for the argument to be optional. (Which you don’t; you never check for nullptr.)
The right thing to do is to use a reference:
void analyzeSeparator(std::string const& input)
However….
There’s an even better way to take string arguments, and that’s to use std::string_view:
void analyzeSeparator(std::string_view input)
No need for pointers, references, or even any consts (because std::string_view is logically const already). Plus, it’s often far more efficient.
Now, one thing your code really needs is comments. Comments are massively important to help people understand what your code is doing. And you’ll probably find that you won’t even remember yourself what your code was doing if you come back to it after a few months.
Don’t add stupid comments like the classic ++i; // increments i. But do try to explain the logic of your code. Anything that isn’t immediately obvious, any clever tricks or reasoning, any assumptions you’ve made… those kinds of things are good candidates for comments.
In this case, it looks like what this function does is:
- Detect repeated separators.
- Detect separators after the decimal point.
Explaining that with a comment would have been a good thing.
Now, this function is made up of 2 for loops and an if block (that hides 3 more loops). Naked for loops are bad practice. You should always use an algorithm wherever possibly; preferably a standard algorithm, but failing that, you can always write one yourself.
So, let’s start with the first loop. What is it doing? Well, it’s replacing all the separators with ~. “Replacing”. So, check out the list of algorithms, and…
(There are old-school and ranges versions of each of these. You should use the ranges versions whenever possible.)
replace() doesn’t really work because it just replaces one character with another. I mean, you could do this:
auto _flattened_ = std::string{input};
std::ranges::replace(_flattened_, ' ', replacer);
std::ranges::replace(_flattened_, '\'', replacer);
std::ranges::replace(_flattened_, ',', replacer);
… but that’s just silly.
What you really need is a is_separator() function:
constexpr auto is_separator(char c) noexcept
{
return c == ' ' or c == '\'' or c == ',';
}
You should have this in any case, because that way you don’t need to repeat the list of separators in multiple places.
With that, you could just do:
auto _flattened_ = std::string{input};
std::ranges::replace_if(_flattened_, is_separator, replacer);
The next for loop looks for cases where there are two adjacent ~. Well, let’s look at the algorithm list and… well, there’s adjacent_find().
The basic version of adjacent_find() just looks for any adjacent duplicates. That’s not great, because it would find the two zeros in 1200.34.
However, there is a version of adjacent_find() that takes a custom predicate. You could just use a lambda to drop in what you actually have:
if (std::ranges::adjacent_find(_flattened_, [](char c1, char c2) { return c1 == '~' and c2 == c1; }))
throw decimal_input_errc::too_many_separators;
But here’s a million-dollar observation… we don’t actually need t replace the separators. We can just use is_separator() directly on the input:
if (std::ranges::adjacent_find(input, [](char c1, char c2) { return is_separator(c1) and is_separator(c2); }))
throw decimal_input_errc::too_many_separators;
You can pull the lambda out to make it easier to read:
auto double_separators = [](char c1, char c2)
{
return is_separator(c1) and is_separator(c2);
};
if (std::ranges::adjacent_find(input, double_separators))
throw decimal_input_errc::too_many_separators;
Finally, the if block at the end searches for a decimal point, and if it finds one, searches for it again (duplicate work!), and then search for any separators after it.
What you could do to avoid the extra work is exploit the fact that if the starting location for a search is past-the-end… well, nothing will be found. So:
if (input.find(replacer, input.find('.')) != std::string_view::npos)
throw decimal_input_errc::separators_after_decimal_point;
However… I would suggest rethinking the strategy here.
The problem is that you do all that work searching for doubled separators on the whole string… even though you only need to do it on the part before the decimal point. If you think about it, you’re treating the before-the-point part of the string and the after-the-point part of the string totally differently. So perhaps you should separate them first… and then do your checks. Something like this:
auto analyzeSeparator(std::string_view input)
{
// Start by assuming there is no decimal point.
//
// So everything is "before" the point, and nothing is after.
auto before_point = input;
auto after_point = std::string_view{};
// Now check to see if there is a decimal point.
if (auto point_position = input.find('.'); point_position != std::string_view::npos)
{
// It has one! So now split the string appropriately.
before_point = input.substr(0, point_position);
after_point = input.substr(point_position + 1); // the "+1" is to skip the point itself
}
// Now check for doubled separators before the point.
auto double_separators = [](char c1, char c2)
{
return is_separator(c1) and is_separator(c2);
};
if (std::ranges::adjacent_find(before_point, double_separators))
throw decimal_input_errc::too_many_separators;
// Then check for *any* separators after the point.
if (std::ranges::any_of(after_point, is_separator))
throw decimal_input_errc::separators_after_decimal_point;
}
Another way to write the first bit is:
// Look for a decimal point first.
auto const decimal_point_position = input.find('.');
// The before-the-point part is easy.
auto const before_point = input.substr(0, decimal_point_position);
// The after-the-point part is a little trickier.
auto const after_point = (decimal_point_position != std::string_view::npos)
? input.substr(decimal_point_position + 1)
: std::string_view{};
The neat thing about writing it that way is everything can be const.
void reconstructInputString(std::string *const input)
{
std::string reconstructedInput {};
for(size_t i=0; i<input->size(); ++i)
{
if(((*input)[i] != ' ') && ((*input)[i] != '\'') && ((*input)[i] != ','))
reconstructedInput.push_back((*input)[i]);
}
*input = reconstructedInput;
}
Here’s another naked for loop. So what are you trying to do here? Well, you’re trying to remove all the separators, right? Let’s look in the algorithm list again and…:
You could use remove_copy_if() to remove all the separators while copying to reconstructedInput, and then copy that back to input. However, it would make more sense to remove the separators from input directly.
Now, for very technical reasons, remove() doesn’t actually remove things from a container. Instead it shuffles the container around so that all the “non-removed” stuff is first, with a bunch of garbage at the end that you then have to erase. Thus, the erase-remove idiom, which looks like this:
auto const [first_removed, last_removed] = std::ranges::remove_if(input, is_separator);
input.erase(first_removed, last_removed);
But as of C++20, there’s a shortcut:
std::erase_if(input, is_separator);
Which means your function is just:
auto reconstructInputString(std::string& input)
{
std::erase_if(input, is_separator);
}
(Which implies the name reconstructInputString isn’t that great, because you’re not really “reconstructing” anything. You’re just stripping the separators.)
void verifyInput(const std::string *const input)
{
/* the outcome will depend on the nature of input *\
|* throws exception if the input is invalid or empty *|
\* returns true if the entry is valid input */
bool presenceOfHyphen {(*input)[0]=='-'}; //a specific identifier to check the presence of '-' (minus) sign at the beginning of the input (implying negative numbers)
unsigned short periodCounter {0}; //an identifier to count the number of period in the input. if > 1 that implies invalid input
unsigned short hyphenCounter {0}; //an identifier to count the number of hyphen in the input. if > 1 that implies invalid input
if(input->empty())
throw 0;
Alright, there are a couple of things wrong here right at the start.
The first problem is that it is bad practice to put all the function variables right at the top of the function. That is archaic C practice; it was never a good idea in C++.
The second problem… which happens because of the first problem… is that the function has a bug. You check for the presence of a hyphen… before checking whether the string is empty. If the string is empty, you have triggered undefined behaviour… which is very bad.
The solution to both problems is simple. Don’t declare variables until you need them. You don’t need presenceOfHyphen until the end of the function.
I also don’t see the point of using unsigned short for the counters. What’s wrong with unsigned int? Or better yet… int. Don’t use unsigned unless you’re doing bit twiddling or modular arithmetic.
The naked for loop is actually two count() operations:
auto const periodCounter = std::ranges::count(input, '.');
auto const hyphenCounter = std::ranges::count(input, '-');
if(periodCounter > 1)
throw decimal_input_errc::invalid_input;
if(hyphenCounter > 1)
throw decimal_input_errc::invalid_input;
Or even just:
if (std::ranges::count(input, '.') > 1)
throw decimal_input_errc::invalid_input;
if (std::ranges::count(input, '-') > 1)
throw decimal_input_errc::invalid_input;
Technically, the way you do it—a single loop for both counts—is more efficient, though there are ways to get the same efficiency with standard algorithms.
double runInputLoop()
{
for(;;)
{
std::string input {};
getline(std::cin, input);
try
{
verifyInput(&input);
reconstructInputString(&input);
std::stringstream ss(input);
double value;
if(ss >> value)
return value;
else
throw -1; //conversion went wrong - probably invalid input.
}
catch (int err_code)
{
printErrorMsg(err_code);
}
}
}
There is a very tricky problem with what you’re doing here, and it stems from the fact that you’re using IOstreams to parse the number. (Incidentally, all you need here is an input stream, so you should be using std::istringstream, not std::stringstream.)
The problem is that you have gone to a lot of trouble to ensure that the number is of the form -?[0-9]*\.?[0-9]* (whether that is really what you intend or not). I presume you intend that the string “1.25” parses as a double with the value of 1¼. You may be surprised to find that it might actually parse as the number 125.
Why? The problem is locales. Streams use locales, and by default, they use the global locale. If no one mucks with the global locale, it will be the standard C or POSIX locale by default. But if someone does much with it… well, in some locales, the period is not a decimal point, it’s just a separator.
What you should be using is std::from_chars(). However, that is a low-level function, and it’s not easy to use. So I would say just stick with the stream parsing for now.
The other issue here has to do with the fact that you’ve mixed up all your error-handling code within your parsing code… which kinda defeats the whole point of exceptions. For all their problems, exceptions have the beautiful feature of being separate from actual logic.
What you should do is rethink the structure of what you’re doing. It’s basically this:
auto main() -> int
{
auto const minN = -3458835.52;
auto const maxN = 9879797.98;
std::cout << "Please enter a number between " << minN << " and " << maxN << " (decimal allowed).\n";
std::cout << "You can use either a space or a comma or \' as separator: ";
// ***
auto val = double{};
auto got_val = false;
while (not got_val)
{
try
{
val = getVDecimal(minN, maxN);
got_val = true;
}
catch (std::exception const& x)
{
std::cout << x.what() << " Try again: ";
}
}
// ***
std::cout << "\nYou have entered: ";
std::cout << std::setprecision(10) << val;
}
Note that when you write it this way, all of the error handling is removed from getVDecimal(). That function now has one and only one job: parsing that value.
You could take the part between the asterisks and put that in a function (or even the stuff between the asterisks and the two couts before)—that would be a good idea, even. But still, the point is that getVDecimal() itself stays pure. One function; one job. Don’t mix the logic all over the place; that’s how you get spaghetti code.
getVDecimal() should do nothing but read the input, try to parse it, and throw on failure. That’s it. No looping. No error handling. That stuff is higher-level logic that should be handled separately.
There’s one more thing I’d suggest you consider, and that is rethinking the way you validate the input. You jump through a lot of hoops going through the input over and over. I would suggest instead making a single pass, using the mental model of a state machine to keep track of where you are in your parsing.
For example (vastly simplified pseudocode just to illustrate):
auto getVDecimal(std::istream& in, value_range const& limits)
{
auto buffer = std::string{};
// The stream sentry sets things up for reading, and does some basic
// checks.
if (auto sentry = std::istream::sentry{in}; sentry)
{
// Let's start by checking for a negative sign.
auto negative = char(in.peek()) == '-';
// If there was a negative sign, eat it.
if (negative)
{
buffer.push_back('-');
in.ignore():
}
// Now we should read a sequence of digits and separators.
//
// We'll stop at either eof or a '.'.
//
// We'll check for doubled separators as we read.
auto last_char_was_separator = false;
while (in.good() and char(in.peek()) != '.')
{
auto c = char(in.peek());
if (not is_digit(c) and not is_separator(c))
// invalid character
if (is_separator(c) and last_char_was_separator)
// doubled separator
// If we got here, it is either a digit or a separator, *and*
// it is not a double separator.
// Keep digits, ignore separators.
if (is_digit(c))
buffer.push_back(c);
last_char_was_separator = is_separator();
// Discard the character.
in.ignore();
}
// Getting here means either:
// 1. we found a '.'; orw
// 2. EOF.
if (in.good())
{
// So we found a '.'.
buffer.push_back('.');
// Now we should read a sequence of digits.
while (in.good())
{
auto c = char(in.peek());
if (not is_digit(c))
// error
buffer.push_back(c);
in.ignore();
}
}
// If we got here, then we successfully parsed:
// 1. an optional '-'
// 2. a sequence of digits with separators (the separators were discarded)
// 3. an optional '.' followed by a sequence of digits (no separators)
// and all of that is in the buffer.
//
// So now it's just a matter of:
auto iss = std::istringstream{buffer};
auto val = double{};
if (iss >> val)
{
if (not limits.contains(val))
throw out of range;
return val;
}
// ... and if we got here, then we failed to parse the buffer string
// as a double. So:
throw invalid input;
}
else
{
// If you catch this error, you should not retry, because the stream
// is bad.
throw std::runtime_error{"invalid input stream"};
}
}
Naturally, this is a very long function, and should be broken up:
auto parse_sign(std::istream& in, std::string& buffer)
{
if (char(in.peek()) == '-')
{
buffer.push_back('-');
in.ignore();
}
}
auto parse_whole_part(std::istream& in, std::string& buffer)
{
auto last_char_was_separator = false;
while (in.good() and char(in.peek()) != '.')
{
auto c = char(in.peek());
if (not is_digit(c) and not is_separator(c))
// invalid character
if (is_separator(c) and last_char_was_separator)
// doubled separator
// If we got here, it is either a digit or a separator, *and*
// it is not a double separator.
// Keep digits, ignore separators.
if (is_digit(c))
buffer.push_back(c);
last_char_was_separator = is_separator();
// Discard the character.
in.ignore();
}
}
auto parse_fractional_part(std::istream& in, std::string& buffer)
{
if (char(in.peek()) == '.')
{
// So we found a '.'.
buffer.push_back('.');
// Now we should read a sequence of digits.
while (in.good())
{
auto c = char(in.peek());
if (not is_digit(c))
// error
buffer.push_back(c);
in.ignore();
}
}
}
auto actually_parse(std::string const& buffer)
{
auto iss = std::istringstream{buffer};
auto val = double{};
if (not (iss >> val))
{
throw invalid input;
}
return val;
}
auto getVDecimal(std::istream& in, value_range const& limits)
{
if (auto sentry = std::istream::sentry{in}; sentry)
{
auto buffer = std::string{};
parse_sign(in, buffer);
parse_whole_part(in, buffer);
parse_fractional_part(in, buffer);
auto const val = actually_parse(buffer);
if (not limits.contains(val))
throw out of range;
return val;
}
else
{
throw std::runtime_error{"invalid input stream"};
}
}
I think that final function is much easier to read, and because you only go through the input once, it will be much more efficient. As mentioned, there is no looping or error handling in there; it’s all handled at a higher level.
Doing it this way is much more complex, and requires learning about input streams (which, warning, they suck). But the benefits are enormous.
