4
\$\begingroup\$

I wanted to write a C function C99/POSIX compliant that read an integer from the user input. I wanted this function to be safe and robust but I feel it is way too complex for such simple task.

I am wondering whether this code is optimal.

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

/**
 * Read an integer from `stdin`.
 * @param min Minimum accepted value
 * @param max Maximum accepted value
 * @param nom Default value
 * @return captured integer
 */
int read_integer(char *text, int min, int max, int nom) {
    int n = nom;
    bool failure = false;

    do {
        printf("%s [%d] ? : ", text, nom);

        // Read user input
        char line[24];
        do {
            if (fgets(line, sizeof(line), stdin) != line || feof(stdin)) {
                exit(EXIT_FAILURE);
                break;
            }
        } while (strchr(line, '\n') == NULL);

        // Default value?
        {
            char *cursor = line;
            while ((*cursor == ' ' || *cursor == '\t') && *cursor != '\0') {
                cursor++;
            }        
            if (*cursor == '\n') {
                return n;
            }
        }

        // Not a number ?
        if (sscanf(line, "%d", &n) != 1) {
            printf("Error: this is not valid entry!\n\n");
            continue;
        } 

        // Not in the range ?
        if (n < min || n > max) {
            printf("Error: value should be between %d and %d!\n\n", min, max);
            continue;
        }

        return n;
    } while(true);
}

int main() {
    do {
        printf("You said %d\n", 
            read_integer("What's the answer", 10, 50, 42));        
    } while(!feof(stdin));
}
\$\endgroup\$
  • 1
    \$\begingroup\$ That code doesn't look to be working like intended. \$\endgroup\$ – πάντα ῥεῖ Nov 20 '19 at 20:59
  • \$\begingroup\$ Could you be more explicit? \$\endgroup\$ – nowox Nov 20 '19 at 21:00
4
\$\begingroup\$

regarding:

if (fgets(line, sizeof(line), stdin) != line || feof(stdin))  

This is unnecessary messy. Suggest:

if( fgets( line, sizeof( line ), stdin )

If any error occurs, the returned value is NULL so the body of the if() will not be entered.

regarding:

if (fgets(line, sizeof(line), stdin) != line || feof(stdin)) {
            exit(EXIT_FAILURE);
            break;

the break; will never be executed because the call to exit() will have already exited the program.

regarding:

while(!feof(stdin));

please read: while(!feof()) is always wrong

regarding:

    return n;
} while(true);

The return is always executed, so this loop will never iterate, looking for a valid input.

regarding:

char *cursor = line;
        while ((*cursor == ' ' || *cursor == '\t') && *cursor != '\0') {
            cursor++;
        }        
        if (*cursor == '\n') {
            return n;
        }

This while() code block will iterate to the end of the array line[], most of the time. the result will be no number will be extracted. Suggest, starting at line[0] to check for isdigit( line[i] ) and if true, then extract the number, perhaps using something like: strtol()

\$\endgroup\$
  • \$\begingroup\$ We generally prefer not to answer questions that contain code that is not working. I would suggested that you should have picked the part of the code that is not working and comment on that. (return n; } while(true) \$\endgroup\$ – pacmaninbw Nov 21 '19 at 16:51

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.