2
\$\begingroup\$

I am studying C++ and after I learned about some functions of the library cctype like isdigit I decided to make a program that validates user input only to be an integer number. So input like 123as, :$ or +.234 are invalid, however I decided to keep the plus or minus sign, i.e., input like -24 or +142 are valid. Below is my code:

#include <iostream> 
#include <string>
#include <cctype>

using namespace std;

bool integer_valid(string s);
int get_integer();

int main()
{
    int number;

    number = get_integer();
    cout << "The integer number is:\t" << number << "\n" << endl;

    system("pause");
    return 0;
}

bool integer_valid(string s)
{
    int i, length_s;
    bool is_valid = true;

    length_s = s.length();

    if (length_s == 0)
        is_valid = false;
    else
    {
        if (length_s == 1)
        {
            if (!isdigit(s[0]))
                is_valid = false;           
        }
        else
        {
            if (s[0] == '+' || s[0] == '-' || isdigit(s[0]) != 0)
            {
                for (i = 1; i < length_s; i++)
                {
                    if (!isdigit(s[i]))
                        is_valid = false;
                }                               
            }
            else
                is_valid = false;
        }           
    }
    return is_valid;
}

int get_integer()
{
    int new_number;
    string number;
    bool is_valid;

    do {
        cout << "Enter an integer number:\t";
        getline(cin, number);
        cout << "\n";

        is_valid = integer_valid(number);

        if (is_valid == false)
            cout << "ERROR. The entered number must be an integer.\n" << endl;
    } while (is_valid == false);

    new_number = stoi(number);

    return new_number;
}

I'm aware that in general is a bad practice to use using namespace std, but since my program has less tan 100 lines of code I think it's really not a big issue here. Moreover, I wonder if there is another (simpler/faster) way to validate integer numbers?

EDIT: Answering to the comment made by Roland Illig, I would say yes. More exactly, any kind of integer should be valid, but as far as I know, int has a defined size, so if I want to valid long integers I should use the type long long int and also the function stoll, right?

Any feedback would be greatly appreciated.

\$\endgroup\$
  • 1
    \$\begingroup\$ Is 111111111111111111111111111111111 a valid integer to you? \$\endgroup\$ – Roland Illig Nov 29 '17 at 4:38
  • \$\begingroup\$ @RolandIllig I've edited my question. \$\endgroup\$ – Xam Nov 29 '17 at 17:12
  • 1
    \$\begingroup\$ "I know doing X is something I shouldn't do but I'm doing it here because Y" is never a good excuse especially when you're still learning the language. Best practices are best practices for a reason. Using them builds good habits and good habits will take you a long way. \$\endgroup\$ – Harald Scheirich Nov 30 '17 at 1:21
4
\$\begingroup\$
  • Using a bool is_valid results in a deeply nested code. You have 5 levels of nesting, with the core of algorithm being hidden at the deepest one. I recommend an early return: as soon as you determine that the string doesn't pass a criteria, return false; immediately.

  • Testing for length_s == 0 is redundant. In this case s[0] is '\0', which is neither a digit nor a sign.

  • Passing std::string as an argument makes a copy. Prefer passing a (constant) reference.

  • Leaving the point of the exercise aside, the best way to validate a number is to just go ahead and directly call std:stoi with the second parameter, e.g.:

        size_t end;
        int number = std::stoi(s, &end);
        if (s[end] != '\0') {
            // The string was not parsed completely. It is not a number.
        }
    
\$\endgroup\$
  • \$\begingroup\$ Is testing the length really redundant? cppreference mentions that accessing s[len] on a non-const string invokes undefined behavior, so I'd rather be careful here. \$\endgroup\$ – Roland Illig Nov 29 '17 at 4:24
  • 1
    \$\begingroup\$ @RolandIllig The question is tagged c++11, where it is well-defined. \$\endgroup\$ – vnp Nov 29 '17 at 4:26
  • \$\begingroup\$ Thanks for your answer. For some reason in my classes we use C++ without mentioning its version. That's the reason why I don't know exactly which things are well defined which things aren't. \$\endgroup\$ – Xam Nov 29 '17 at 17:16
3
\$\begingroup\$

Avoid system("pause")

It's a security risk, not portable, and introduces unnecessary overhead.

Variables should use the proper types

The return type for std::string::length() is std::string::size_type (which is usually an unsigned type), so that should be the type for length_s, not int.

Similarly, i should be of type std::string::size_type because:

  1. you are comparing it to length_s on the line for (i = 1; i < length_s; i++). The two variables should both be unsigned for proper comparison, and preferably the exact same type.
  2. you are using it as the argument type for std::string::operator[], which uses std::string::size_type as the argument type.

I would still avoid using namespace std

On Stack Overflow, I once saw an even shorter program which failed to compile due to using namespace std -- the programmer attempted to define a function int plus(int, int) which conflicted with std::plus. Your program may have less than 100 lines of code, but that just means there aren't too many instances in which you have to qualify a name with std::. Plus, it's best to get into the habit of not using using namespace std for when you write longer programs.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. Why shouldn't I use the type int for the variable i? It's the counter. \$\endgroup\$ – Xam Nov 29 '17 at 17:20
  • 1
    \$\begingroup\$ @Xam You are comparing i to length_s, so they should both be unsigned. Your compiler should warn you about a signed/unsigned comparison if i is an int but length_s is std::string::size_type. I've updated my answer to explain this more fully. \$\endgroup\$ – Null Nov 29 '17 at 17:27
  • 1
    \$\begingroup\$ @Null, compilation errors are not that evil as ADL jokes. \$\endgroup\$ – Incomputable Nov 29 '17 at 17:55
2
\$\begingroup\$

Solution one:

You're already using std::stoi, did you know that it throws an exception if it can't convert to an int? It already does the validation for you, use it like:

do {
    ... read input ...

    try {
        number = std::stoi(s);
    } catch (const std::exception& e) {
        std::cout << "ERROR. The entered number must be an integer.\n";
        valid = false;
    }
} while (!valid);

Solution two:

If you just check if (std::cin >> number) (or in the condition of a loop) it'll fail if it couldn't read an int. No extra validation needed.

Other points:

  • Use \n instead of std::endl unless you explicitly need to flush the stream (you don't)
  • Never use using namespace std, even in short programs
  • Check a bool using myBool == false is strange to read, prefer !myBool
\$\endgroup\$

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.