Function to prompt user for input

Question

The following program calls a function, which prompts the user for input. The function then returns one of three enum constants; INPUT_SUCCESSFUL, INPUT_TOOLONG, or INPUT_FAILED. If successful, it prints the text entered by the user. Otherwise, it prints an error message, and takes the appropriate action.

The function contains three parameters. The first one is a constant character array, which gets passed the text that will be used to prompt the user. The second is a character array, in which the text entered by the user will be placed. And the third is the size of the character array.

I have the following questions...

1) Would it have been better to let the called function handle the printing of error messages, exiting the program, etc, instead of the main function?

2) Would it have been better to pass a pointer to the function so that it could allocate the appropriate memory, instead of passing it a fixed array?

3) Any potential issues with the program or improvements that can be made to the code?

The program was compiled as follows...

gcc -std=c99 -Wall -Wextra -Wpedantic main.c user_input.c

...and received only warnings about unused parameters argc and argv. Here are the files...

main.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "user_input.h"

#define MAX_STR_LEN 50

int main(int argc, char *argv[])
{
    char user_input[MAX_STR_LEN + 2]; /* +2 for newline character (\n) and terminating null character (\0) */
    input_status status;

    while (1)
    {
        status = get_user_input("Enter your name: ", user_input, sizeof(user_input));

        if (status == INPUT_FAILED)
        {
            fprintf(stderr, "Unable to read user input\n");
            exit(EXIT_FAILURE);
        }

        if (status == INPUT_TOOLONG)
        {
            fprintf(stderr, "Entry exceeds %d characters\n", MAX_STR_LEN);
            continue;
        }

        if (strlen(user_input) == 0)
        {
            fprintf(stderr, "Entry is missing\n");
            continue;
        }

        if (user_input[0] == 0x18) /* user entered Ctrl+X (^x) to cancel */
        {
            fprintf(stderr, "User cancelled\n");
            return 0;
        }

        break;
    }

    printf("User entered:  %s\n", user_input);

    return 0;
}

user_input.h

#ifndef USER_INPUT_H
    #define USER_INPUT_H

    typedef enum
    {
        INPUT_SUCCESSFUL,
        INPUT_TOOLONG,
        INPUT_FAILED
    } input_status;

    void flush_input();

    input_status get_user_input(const char *prompt, char *user_input, size_t size);

#endif

user_input.c

#include <stdio.h>
#include <string.h>
#include "user_input.h"

void flush_input()
{
    int c;

    while ((c = getchar()) != '\n' && c != EOF)
        /* skip it */ ;
}

input_status get_user_input(const char *prompt, char *user_input, size_t size)
{
    printf("%s", prompt);

    if (fgets(user_input, size, stdin) == NULL)
        return INPUT_FAILED;

    char *pNewline;
    if ((pNewline = strchr(user_input, '\n')) == NULL)
    {
        flush_input();
        return INPUT_TOOLONG;
    }

    *pNewline = '\0';

    return INPUT_SUCCESSFUL;
}

Answer 1

It seems fairly sane; I only picked up a couple of things:

Don't ignore errno

You do check for failure, which is good; but you ignore errno. You should use it, and/or perror, to get more detailed failure information.

Assign-in-condition

There's not a compelling reason to do this:

char *pNewline;
if ((pNewline = strchr(user_input, '\n')) == NULL)

Just assign pNewLine where it's declared, not where it's used in the condition.

Answer 2

• 1) Would it have been better to let the called function handle the printing of error messages, exiting the program, etc, instead of the main function?

  It is always better to separate error handling from the rest of the code. But you could have placed the error handling in a separate function.

  Related to this, you could have created a more meaningful loop condition in the caller than while(1). Also, the presence of continue in a C program is always "code smell"; it is a dead certain indication that a loop can be rewritten in better ways. There is a very sound programming rule saying that C code should never jump non-conditionally upwards - this applies to continue and goto both.

  For example you could create a separate function for error handling the type input_status, which could either be part of user_input.h/user_input.c, or defined in the caller, whatever makes most sense program design-wise.

  bool input_status_ok (input_status status)
  {
    if(status == INPUT_FAILED)
    {
      fprintf(stderr, "Unable to read user input\n");
      exit(EXIT_FAILURE);
      return false; // never executed but might block potential compiler warnings
    }
    if (status == INPUT_TOOLONG)
    {
      fprintf(stderr, "Entry exceeds %d characters\n", MAX_STR_LEN);
      return false;
    }
    return true;
  }
  

  (In case of a whole lot of different error codes, we could perhaps replace the if chain with a switch.)

  With that out of the way of your loop, you can have one loop checking the errors returned by the function, and another, outer loop for checking the data contents:

  bool user_cancel = false;
  
  while (!user_cancel)
  {
    do 
    {
      status = get_user_input("Enter your name: ", user_input, sizeof(user_input));
    } while( !input_status_ok(status) );
  
    if (user_input[0] == CANCEL) /* user entered Ctrl+X (^x) to cancel */
    {
      fprintf(stderr, "User cancelled\n");
      user_cancel = true;
    }
  }
  

  We've now separated error handling of the function result from error handling of the data contents. There are no icky non-conditional branches and the code is overall easier to read.

• 2) Would it have been better to pass a pointer to the function so that it could allocate the appropriate memory, instead of passing it a fixed array?

  No, it is always better to leave allocation to the caller when you have that option. That way, you separate memory allocation from algorithm which is better program design - meaning your function will only do its designated task, and it won't care if the memory used is allocated statically, on the stack or on the heap.

---

3) Any potential issues with the program or improvements that can be made to the code?

• Avoid "magic numbers" - add something like #define CANCEL 0x18u instead of using the magic number 0x18 in the middle of the code.

• Never write functions with an empty parenthesis for the parameter list in C, such as void flush_input(). This is obsolete style and has poor type safety, since it means "function that accepts any parameters". C is different from C++ here, in C++ the empty parenthesis is encouraged and equivalent to (void).

• Avoid assignment inside conditions. This is problematic for many reasons:

  • Assignments introduce an extra side-effect that might have dependencies in relation to the right-hand side of the assignment.
  • The classic == vs = bug. All modern compiler can check for this, but if you never write = inside conditions you don't need to worry. (Better yet, you won't have to deal with people who's been living under a rock since the 1980s and therefore still suggest that you should use the unreadable Yoda conditions )
  • Mashing as many operations as possible into a single line makes the code harder to read.

  Instead, simply write:

  char *pNewline = strchr(user_input, '\n');
  if (pNewline == NULL)
  

• You should place all library #includes in the h file. This documents all library dependencies to the user of your code, who should only need to read the public h file.

Answer 3

Incorrect test

Not finding a '\n' is a "too long a line" when strlen(user_input) + 1 == size

// not quite the right test
if ((pNewline = strchr(user_input, '\n')) == NULL) {
    flush_input();
    return INPUT_TOOLONG;
}

Not finding a '\n' can occur when that was all that was left in stdin and end-of-file occurred without the last character as a '\n'.

size_t size_used = strlen(user_input) + 1;
if (size_used == size) {
    flush_input();
    return INPUT_TOOLONG;
}

Advanced issue not handled: Not finding a '\n' can occur when a null character was read as strchr(user_input, '\n') stops at the read null character and not the appended one.

Spinning CPU cycles

No need to run down the length of the string to find it length. Simply test first element.

// if (strlen(user_input) == 0)
if (user_input[0] == 0)