Short header file for safe CLI input handling in C

Proper input handling is a pain in the butt in C, so as part of a project of mine, this header is used to handle reoccurring command line input tasks such as input loops, reading lines and tokenizing strings in a safe and robust way. Would be great if some C veterans could share their opinion about potential flaws.

You can check the updated version here.

```
#include
#include
#include
#include
#include
#include

#define TRUE 1

/*
* Contains functions for input related operations
*/

/// Wraps fgets to handle errors and safely replaces the newline
/// \param line - The buffer which holds the string
/// \param length - The size of the buffer
void get_line(char *line, const int length) {
if (fgets(line, length, stdin)) {
char *newline = strchr(line, '\n');
if (newline) {
*newline = '\0';
}
} else {
fprintf(stderr, "Could not read line from stdin!");
exit(EXIT_FAILURE);
}
}

/// Tokenizes a line into EXACTLY number_of_tokens tokens using the " " delimiter. Excess tokens are omitted. This
/// function will alter the line parameter, so be sure to pass a copy if you want to preserve it.
/// \param line - The string to be processed
/// \param tokens - The buffer holding the tokens
/// \return EXIT_FAILURE if the supplied line does not contain enough tokens.
int get_tokens(char *line, char **tokens, const size_t number_of_tokens) {
for (size_t i = 0; i INT_MAX) {
fprintf(stderr, "%ld is greater than INT_MAX\n", value);
} else if (value SIZE_MAX) {
fprintf(stderr, "%ld is greater than SIZE_MAX\n", value);
} else {
*size = (size_t) value;
return EXIT_SUCCESS;
}
return EXIT_FAILURE;
}

/// Prompts for an integer value once
/// \param value - A pointer to the integer
/// \return EXIT_FAILURE if the supplied input doesn't match
int get_integer(int *value) {
char buffer[BUFSIZ];
get_line(buffer, BUFSIZ);

char **tokens = malloc(sizeof(char*))

I see some things that I think could help you improve your code.

Eliminate unused definitions

The top of the code includes this line:

#define TRUE 1

However, it's not used within the code and should be removed. Also, rather than defining your own values for TRUE and FALSE, use #include instead.

Don't rely on implementation-defined values

It seems that you are using EXIT_FAILURE and EXIT_SUCCESS as return values for your functions. This is an error because the value of these constants are implementation defined. This means that lines like this:

if (string_to_integer(tokens[0], &result)) {

May or may not work as you intend because the value of EXIT_SUCCESS is not required to be 0. In short, don't use EXIT_FAILURE and EXIT_SUCCESS except perhaps as argument to exit().

Don't allocate memory if you don't need to

This code calls malloc several places but doesn't always match each one with a call to free. This means that the routines are leaking memory. For instance, the existing implementation of get_integer looks like this:

int get_integer(int *value) {
    char buffer[BUFSIZ];
    get_line(buffer, BUFSIZ);

    char **tokens = malloc(sizeof(char*));
    if (get_tokens(buffer, tokens, 1)) {
        return EXIT_FAILURE;
    }

    int result;
    if (string_to_integer(tokens[0], &result)) {
        return EXIT_FAILURE;
    }

    *value = result;
    free(tokens);
    return EXIT_SUCCESS;
}

The problem, however, is that if the call to malloc succeeds, but the call to string_to_integer fails, the allocated memory is not freed and worse, the pointer to it is lost. There's no need to call malloc here, though as shown in the next suggestion.

Don't allocate memory when you don't need to

The memory leak shown in the previous suggestion can easily be addressed by simply not explicitly allocating memory. One way to rewrite this code would be this:

int get_integer(int *value) {
    char buffer[BUFSIZ];
    get_line(buffer, BUFSIZ);
    char *tokens;
    if (get_tokens(buffer, &tokens, 1)) {
        return 1;
    }
    int result;
    if (string_to_integer(tokens, &result)) {
        return 2;
    }
    *value = result;
    return 0;
}

This also uses different constants 1 and 2 so that the calling code can determine which error occurred. In real code, one would use named constants. It appears that the code has taken care to only alter value if the input succeeds. If this is not important, one can read the value directly into value.

Use const where practical

The string passed to string_to_size_t and to string_to_integer is not modified by the function, so it should be declared as const:

int string_to_size_t(const char *string, size_t *size);

Think of the user

The error messages might make sense to a C programmer, but are not particularly helpful otherwise. For example, this line:

fprintf(stderr, "%ld is greater than INT_MAX\n", value);

Seems to assume that the user knows what INT_MAX is and what value it has, which are both unlikely to be true for those who aren't C programmers familiar with your particular compiler.

Choose better names

By the name prompt_for_size_t and prompt_for_integer, I would expect them to actually provide a prompt, but they actually don't. What they do instead is only provide some message if there is some flaw detected in the input. Also size_t names a type, but integer does not, so they're not consistent. You might consider passing in a const char * string so that it could actually be used as a prompt. Then one could use it like this:

prompt_for_size_t("Please enter a positive integer");

Reconsider the interface

Some of the functions print to stderr if they encounter an error and some do not. I'd suggest that it might be better and more consistent to have most functions simply return an error code and let the calling function provide any prompts, if needed. If you find that you are often using the same messages for certain kinds of input, you can create wrapper functions such as string_to_size_t to handle that. It might also be useful to be able to ask for an integer in a particular range, so an interface passing in low and/or high values might be handy.

StackExchange Code Review Q#146904, answer score: 3