3
\$\begingroup\$

I am a beginner, who is just learning the language (C++) from C++ Primer. I have been trying to build a side pico-project by writing functions for validation of user input for an signed double. The code is provided below. My questions:

  1. Are there any logical errors in the code (I have tested several times with invalid inputs and the code works, but I might have missed many probabilities - hence, asking for expert advice).
  2. Are there any ways to optimize the code, anywhere?
  3. What other better ways to handle user inputs' errors?
  4. Are there any built-in library functions that deals with invalid inputs from the end-users (asking this to know - if I am trying to re-invent the wheel - even if it was for academic purpose)?

EDIT: INTENTION OF THE CODE

6546y.6     invalid
2,45,556    valid
-2'34.89    valid
--23,89     invalid
3,,4.6      invalid
78-23.78    invalid
-,,         invalid
-345.4,56   invalid

CODE GOES HERE:

#include <iostream>
#include <string>
#include <sstream>
#include <iomanip>

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;
    }
}

void analyzeSeparator(const std::string *const input)
{
    //Further Analysis starts from here

    std::string _flattened_ {}; //just bear with the name, damn it!
    const char replacer {'~'}; //can be bloody anything

    for(size_t i=0; i<input->size(); ++i)
    {
        if(((*input)[i] == ' ') || ((*input)[i] == '\'') || ((*input)[i] == ','))
            _flattened_.push_back(replacer);
        else
            _flattened_.push_back((*input)[i]);
    }

    for(size_t i=0; i<_flattened_.size()-1; ++i)
    {
        if(_flattened_[i] == replacer)
        {
            if(_flattened_[i] == _flattened_[i+1])
                throw -2;
        }
    }

    if(_flattened_.find('.') != std::string::npos)
    {
        if(_flattened_.find(replacer, _flattened_.find('.')) != std::string::npos)
            throw 2;
    }
}

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;
}

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;

    if(input->find_last_not_of("-.0123456789 \',")!=std::string::npos)
        throw 1;

    analyzeSeparator(input);

    for(auto ch : *input)
    {
        if(ch=='.') //counting numbers of period
            ++periodCounter;
        if(ch=='-') //counting numbers of hyphen
            ++hyphenCounter;
    }

    //std::cout << std::endl << "pC: "<<periodCounter <<"\t"<< "hC: "<<hyphenCounter <<"\t"<< "pOH: "<<presenceOfHyphen <<std::endl; //diagnostic statement

    if(periodCounter > 1)
        throw -1;

    if(hyphenCounter > 1)
        throw -1;

    if(hyphenCounter == 1 && !presenceOfHyphen)
        throw -3;
}

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);
        }
    }
}

double getVDecimal(const double minN, const double maxN)
{
    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: ";

    for(;;)
    {
        double value {runInputLoop()};
        if(value >= minN && value <=maxN)
            return value;
        else
            std::cout << "Input out of range. Try again: ";
    }
}

int main()
{
    double val {getVDecimal(-3458835.52, 9879797.98)};
    std::cout << "\nYou have entered: ";
    std::cout << std::setprecision(10) << val;

    return 0;
}

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I think it would help if you did a bit more to explain what input you consider valid and what you don't. \$\endgroup\$ Aug 5 at 18:09

3 Answers 3

4
\$\begingroup\$

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:

  1. 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?
  2. Write the tests.
  3. 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:

  1. Detect repeated separators.
  2. 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.

\$\endgroup\$
10
  • \$\begingroup\$ The way I see it - I am probably thinking in a 'C' way rather than the '++' way. Correct me if I am wrong. Also, I am yet to comprehend - what is meant by - C++ is a STRONGLY TYPED language. \$\endgroup\$
    – SogaBan
    Aug 6 at 6:35
  • \$\begingroup\$ And you have used lots of 'auto'. Just being sceptical - isn't that being lazy OR should it be my modus operandi? When should I use auto and when NOT? \$\endgroup\$
    – SogaBan
    Aug 6 at 7:20
  • \$\begingroup\$ Mm, I’ve seen a lot of code by people stuck in C mode, and this doesn’t really have that vibe. In fact, it’s pretty close to what many C++ coders would write when they’re bashing out a quickie, and too lazy/busy to make proper types. The thing with doing things the C++ way is that it often costs a lot upfront… but pays off enormously over the long term as code get bigger and more complex. So, for simple, one-off projects, it can feel burdensome… but for large projects or libraries, you’ll soon realize the C++ way makes things massively better. \$\endgroup\$
    – indi
    Aug 7 at 22:00
  • \$\begingroup\$ Strong typing means a lot of things, but very basically: C++ is very strict about the types of variables, and does not (usually) silently convert from one type to another. The things a variable can do are restricted according to its type: so you can create a type for ID numbers that is different from a type for regular numbers… even though they’re both technically just numbers… where you can do arithmetic with the plain numbers, but not with ID numbers (which wouldn’t make sense). \$\endgroup\$
    – indi
    Aug 7 at 22:00
  • \$\begingroup\$ A good example where strong typing is very useful is physical quantities. You could have a type for lengths, and a type for masses. Both are just numbers. But while you can add length to length, and mass to mass, it makes no sense to add length to mass. What would “2 cm + 5 kg” even equal? But you can multiply/divide lengths and masses: “5 kg ÷ 2 cm” is 2.5 kg/cm, or 250 kg per metre. A strong type system can enforce these behaviours, and even do automatic unit conversions… so you’ll never mix up N·s with lbf·s. \$\endgroup\$
    – indi
    Aug 7 at 22:01
2
\$\begingroup\$

Let the standard library validate the input for you

The standard library does provide ways to parse a number from a string and to check whether the whole string was a valid number. Up to C++17, your best option is to use strtod(), and use the optional end pointer to check that the whole input was parsed:

bool verifyInput(const std::string& input) {
    char *end;
    strtod(input.c_str(), &end);
    return end == input.c_str() + input.size();
}

The end pointer will point to the first non-valid character in the input. So if it points to input.end(), that means all of the input was valid. Since C++17 you can also use std::from_chars(), but not all standard library implementations support it yet at this moment.

Another possibility is to use a regular expression to check if a strings is a valid floating point number, like shown in this post. However, using strtod() or std::from_chars() might be better, and as a benefit those also return the actual double.

Error handling

If you want to use exceptions, the proper thing to do is to create your own exception class(es) that inherit from one of the standard exception types.

However, don't use exceptions for things that are not exceptional. If you have a function verifyInput(), you already expect that some inputs will not be valid. This function should just return a bool indicating whether the input was correct or not. If you really want to return more information, consider returning a std::error_code (see this post to links to how to make your own).

\$\endgroup\$
1
\$\begingroup\$

I find this code rather difficult to follow. Right now, it seems to intermix code for determining whether a string constitutes a valid floating point value, with code for determining...other stuff you care about (something like allowing a number of floating point values together, maybe?)

I'd start with one that just determines whether a string is a valid floating point value. For the moment I'm assuming that means something like:

[-+]<digits>[.<digits>][[eE][-+]digits]

Almost all the parts of this are optional individually, but you do need at least one digit either in the "main" number (the part before the exponent). So 1e3 is valid, but just e3 is not (and likewise, either .1 or 1. is valid, but just . is not).

Code for that much might look something like this:

#include <ctype>
#include <string>
#include <iostream>

bool isValidDouble(std::string const &input) {
    unsigned pos = 0;
    unsigned digits = 0;
    bool exponent = false;

    while (std::isspace(input[pos])) {
        pos++;
    }

    if (input[pos] == '-' || input[pos]=='+') {
        pos++;
    }

    while (isdigit(input[pos])) {
        digits++;
        pos++;
    }

    if (input[pos] == '.') {
        pos++;
    }

    while (std::isdigit(input[pos])) {
        digits++;
        pos++;
    }

    if (digits == 0) {
        return false;
    }

    if (input[pos] == 'e' || input[pos] == 'E') {
        if (digits == 0) {
            return false;
        }
        exponent = true;
        pos++;
    }

    if (exponent && input[pos] == '-' || input[pos] == '+') {
        pos++;
    }

    while (exponent && std::isdigit(input[pos])) {
        pos++;
    }

    return pos == input.length();
}

int main() {
    std::string goodInputs[] = {
        "    1234",
        "   1.2",
        "\t0e2",
        "0.1e2",
        "1.",
        ".1",
    };

    std::string badInputs[] = {
        "e2",
        ".",
        ""
    };

    for (auto const &test : goodInputs) {
        if (!isValidDouble(test)) {
            std::cout << "Test failed for :" << test << "\n";
        }
    }

    for (auto const &test : badInputs) {
        if (isValidDouble(test)) {
            std::cout << "Test failed for : " << test << "\n";
        }
    }
}

I'm not entirely sure I understand what other validation you want, so I'll leave it at that for now.

\$\endgroup\$
2
  • \$\begingroup\$ I also wanted to add provisions for separator. For example, -3,4,89,000.7789 will be considered a valid input; while -3,56,000.98,9876 will be not. Using your code base, I got an error on this one: Test Failed for: -4,34,678 And salute to your so lucid and simple algorithm!! Will I ever reach that place? I wonder... \$\endgroup\$
    – SogaBan
    Aug 5 at 23:07
  • \$\begingroup\$ @SogaBan: Ignoring commas should be pretty easy--pretty much while (isdigit(ch)) becomes while (isdigit(ch) || ch == ','). There is a bit of trickiness, as that will let something like ,9 through, where you probably only want to allow a comma that's both preceded and followed by a digit. \$\endgroup\$ Aug 6 at 1:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.