C function to read only numeric values

Question

I'm learning C and as exercise I'm trying to implement a custom function to parse a input from user and return successfully only if the input is a number.
These are my files, how could I improve it?

my_main.c

#include "my_utils.h"

void main() {
    int a;
    char num[10]; 
    size_t l = sizeof(num); 
    a = read_int(&a, num, l, message);
    printf("The number is %d", a);
}  

my_utils.h

#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int read_int(int *a, char *num, size_t l, const char *message) {
    int err;
    size_t length;
    do {
        if (message)
            printf(message);
        fgets(num, l, stdin);
        length = strlen(num);
        err = 0;
        int i;
        for (i = 0; i < l; i++) {
            if (!isdigit(num[i]) && num[i] != '\n' && num[i] != '\0' && num[i] != EOF) {
                if(i == 0 && num[i] == '-') //-123 is a valid input, 1-23 is not
                    continue;
                printf("Only numbers allowed. Try again\n");
                err = 1;
                if (length > 0 && num[length - 1] != '\n') { //stdin flush
                    int c;
                    while ((c = getchar()) != '\n' && c != EOF)
                        ;
                }
                break;
            }
            if (num[i] == '\n' && num[i + 1] == '\0') {
            break;
            }
        }
    } while (err != 0);
    *a = atoi(num);
    if (length > 0 && num[length - 1] != '\n') {
        int c;
        while ((c = getchar()) != '\n' && c != EOF)
            ;
    }
    return *a;
}

Maybe (surely?) is overkill to handle only a int input, but I'd like to know if there are best practice or whatever a newbie should know to improve. Thanks

Answer 1

Definition of function main

The definition of the function main should start with

int main( void )

instead of:

void main()

The return type of void is illegal in ISO C, unless your platform happens to support it as an extension. However, even if your platform does support void, using it is generally not recommended, because otherwise, your code may not be portable to other platforms.

See the following Stack Overflow question for further information:

What should main() return in C and C++?

Add include guards to header files

You should generally add include guards to header files, for example

#ifndef MY_UTILS_H_INCLUDED
#define MY_UTILS_H_INCLUDED

at the start of the file, and then

#endif

at the end of the file.

This will ensure that the header file will only be included once per translation unit (.c source code file). Otherwise, you may get errors if the same header file is included more than once in the same translation unit. See the following question for further information:

Why are #ifndef and #define used in C++ header files?

On some platforms, you can simply write #pragma once at the start of the file, in order to achieve the same effect. However, this is not guaranteed to work on all platforms. See the following question for further information:

#pragma once vs include guards?

Functions should be defined in source files, not header files

In your posted code, you are defining the function read_int in a header file. However, this is generally a bad idea, because if this header is included by more than one translation unit (.c source file), then your program will likely fail to compile/link, due to violating the one-definition rule.

For this reason, functions should generally be defined in .c source files and not in header files. Header files should generally only contain function declarations, but not definitions.

I therefore suggest that you move the definition of the function read_int to a file with the name my_utils.c and that you compile that file together with my_main.c. The file my_utils.h should only contain a declaration of the function read_int:

int read_int(int *a, char *num, size_t l, const char *message);

Don't call printf using a format string from an unknown source

The line

printf(message);

is dangerous, because message is interpreted as a pointer to the format string. This string is supplied by the caller and it is unclear whether it may contain % characters, which have a special meaning in a printf format string. For example, if the format string happens to contain %s, then the function printf will attempt to de-reference the non-existant pointer parameter, which will likely cause the program to crash.

For this reason, it would be safer to write

printf( "%s", message );

or

fputs( message, stdout );

instead. That way, if the string happens to contain a %, this character will not have any special meaning; it will be printed like any other character.

Always check the return value of fgets

It is possible for the function fgets to fail, for example due to an I/O error or if the user triggers end-of-file on the terminal/console (for example by pressing CTRL-D on Linux or CTRL-Z on Windows). Therefore, you should generally always check the return value of fgets and, if an error occurred, act accordingly.

If fgets returns NULL, then num is not guaranteed to point to a null-terminated string. If you call

length = strlen(num);

and num does not point to a null-terminated string, then your program will invoke undefined behavior (i.e. your program may crash).

The parameters of the function read_int are unnecessarily cumbersome

You have defined the function read_int with the following parameters and return type:

int read_int(int *a, char *num, size_t l, const char *message)

The function is passing back the result value both as a return value and via the pointer parameter a. In other words, the function is passing the same value back to the calling function twice, although once would be sufficient. I recommend that you only use the return value. That way, you can eliminate the parameter a.

Also, there is no reason for the function read_int to use a memory buffer supplied by the caller. If the function uses its own memory buffer instead, then you can also eliminate the parameters num and l.

Use strtol instead of atoi

I recommend that you use the function strtol instead of atoi. This has the following advantages:

1. The function strtol will tell you if the value is outside of the representable range of the integer data type, whereas the function atoi will invoke undefined behavior in that case.

2. The function strtol will tell you how many characters were successfully converted, by passing an optional pointer to the first character that was not parsed. That way, it is no longer necessary for you to call isdigit on every single character in order to determine whether the input is valid. Also, this will automatically allow the digits to be prefixed with + or - and you will no longer have to handle these cases yourself.

Code example

Here is the definition of my function get_int_from_user, which I have been using regularly and is intended to do the same thing as your function read_int. It follows all the advice mentioned above.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include <limits.h>
#include <errno.h>

//This function will attempt to read one integer from the user. If
//the input is invalid, it will automatically reprompt the user,
//until the input is valid.
int get_int_from_user( const char *prompt )
{
    //loop forever until user enters a valid number
    for (;;)
    {
        char buffer[1024], *p;
        long l;

        //prompt user for input
        fputs( prompt, stdout );

        //get one line of input from input stream
        if ( fgets( buffer, sizeof buffer, stdin ) == NULL )
        {
            fprintf( stderr, "Unrecoverable input error!\n" );
            exit( EXIT_FAILURE );
        }

        //make sure that entire line was read in (i.e. that
        //the buffer was not too small)
        if ( strchr( buffer, '\n' ) == NULL && !feof( stdin ) )
        {
            int c;

            printf( "Line input was too long!\n" );

            //discard remainder of line
            do
            {
                c = getchar();

                if ( c == EOF )
                {
                    fprintf( stderr, "Unrecoverable error reading from input!\n" );
                    exit( EXIT_FAILURE );
                }

            } while ( c != '\n' );

            continue;
        }

        //attempt to convert string to number
        errno = 0;
        l = strtol( buffer, &p, 10 );
        if ( p == buffer )
        {
            printf( "Error converting string to number!\n" );
            continue;
        }

        //make sure that number is representable as an "int"
        if ( errno == ERANGE || l < INT_MIN || l > INT_MAX )
        {
            printf( "Number out of range error!\n" );
            continue;
        }

        //make sure that remainder of line contains only whitespace,
        //so that input such as "6abc" gets rejected
        for ( ; *p != '\0'; p++ )
        {
            if ( !isspace( (unsigned char)*p ) )
            {
                printf( "Unexpected input encountered!\n" );

                //cannot use `continue` here, because that would go to
                //the next iteration of the innermost loop, but we
                //want to go to the next iteration of the outer loop
                goto continue_outer_loop;
            }
        }

        return l;

    continue_outer_loop:
        continue;
    }
}

This code was copied from this answer of mine to another question. See that answer for further explanation of my code.

Answer 2

Late to the review

Why 10 in char num[10];?

If trying to accommodate all 32-bit int, including "-2147483648\n", the input buffer needs to be larger - at least 13. Even larger for error detection.

As it is good to

• oversize the input buffer to accept reasonable variants like " -00001234567890 \n",
• oversize the input buffer to detect extra junk,
• scale the buffer to the size of int,
• avoid naked magic numbers,
• avoid huge buffers like char num[1000000] for something that only needs a few dozen,

... consider

#define INT_BUF_SIZE (sizeof(int) * CHAR_BIT)  // About 3x min size needed
char num[INT_BUF_SIZE];

For a common 32-bit int, this will be 32 to handle input like "-2147483648\n" along with some leading spaces/zeros and trailing white-spaces.

True that "valid" input could be "000...million more zeros... 000123", yet at some point such excessively long input should be considered hostile and not valid.

I recommend using a 2-3x buffer of the the longest standard input.

Answer 3

<ctype.h> functions accept int

The functions in <ctype.h>, such as isdigit() accept only positive integers and the constant EOF as inputs. On platforms where char is a signed type, converting directly to int can be Undefined behaviour. Convert to unsigned char first, using a cast or by reading through a unsigned char*.

l is a terrible name for a variable

l is easily confused with 1 (or I). Prefer more descriptive names, particularly in public interfaces.