Reading input from stdin

Question

I read user input from stdin in plain C. The problem is that I want a sane implementation that is robust to errors and restricts the user to a certain input and doesn't suck in terms of complexity. The function get_strings() reads input char by char as long there is no new line (\n), no EOF and all chars are passing the isalpha() test. But I want to keep spaces.

Some points that (I think) deserve special attention during the review:

- I would love to get rid of the outer while loop which is basically testing wether the user just pressed Enter without any meaningful input.

- Do I even need to use ferror()? fgetc already returns EOF when something did go wrong, but I will just stop reading from the stream instead of telling the user something went wrong.

- It doesn't happen everytime but it happens: A \n stays in the stdin stream and the next time I want to get meaningful input fgetc() just gets skipped. It doesn't matter here, but it does when I am asking single character Yes/No questions. I can only get rid of this problem with a construct that clears out all previous things that are left in stdin. See the second codeblock for this.

So, am I handling this totally wrong? Are there better practices to embrace? It looks really clunky to me, and clunky is always bad.

/** @brief Contains the dictionary */
static char **strings = NULL;

/** @brief Helps with containing the dicionary */
static char *string;

/* Reads input char by char with fgetc() */
static char *get_strings() 
{
    char *string = NULL;
    char ch;
    size_t len = 0;
    while (string == NULL && ch != EOF) {
    while (EOF != (ch = fgetc(in_stream)) && ch != '\n') {
        if (ch != ' ' && isalpha((int)ch) == 0) {
            fprintf(stderr, "Only [a-z] is a valid input. | \t"
                                "| Input another or end with CTRL+D: ");
            continue;
        }
        string  = (char*) realloc(string, len+2);
        if (string == NULL) {
            bail_out(EXIT_FAILURE, "realloc(3) failed");
        }
        string[len++] = toupper(ch);

        if (len >= MAX_DATA) {
            bail_out(EXIT_FAILURE, "Input too long\n");
        }
    }
    if (ferror(in_stream)) {
        bail_out(EXIT_FAILURE, "Error while reading from stream");
    }
    }

    if(string) {
        string[len] = '\0';
    } else {
        printf("\nFinished dictionary...\n");
    }
    printf("Added string: %s | Input another or end with CTRL+D: ", string); 
    return string;
}

/* Saves the returned strings from get_strings() in a linked list */
static void read_dict()
{
    int index;
    for (index = 0; (string = get_strings()); ++index) {
        if (string[0] == '\0') continue; 
        strings = (char**) realloc(strings, (index+1)*sizeof(*strings));
        if (strings == NULL) {
            bail_out(EXIT_FAILURE, "realloc(3) failed");
        }
        strings[index] = string;
    }

    /* Take a note of how many entries we have yet. */
    dict_size = index;
}

Second CodeBlock with a more simple case:

while(1) {
    char tmp;
    printf("Please enter your guess [a-z]: ");
    guess = fgetc(stdin);
    /* Jump back to start of loop */  
    if (guess == '\n') {
        continue;
    }

    /* HERE IS THE CLEAR FOR STDIN
       This part really just eats all remaining \ns from the user,
       so that later inputs can start uninterrupted. Can I get rid of
       it in some better way? */
    while((tmp = getchar()) != '\n' && tmp != EOF);

    if(!isalpha(guess)) {
        fprintf(stderr, "Enter a valid letter [a-z]!\n");
        continue;
    }
}

Answer 1

Architecture

stdin is usually line buffered. So nothing is given to fgetc() until the user hits Enter. OP code will give multiple error messages with input like "Hello 123". Better to separate user input from input validation. Read the line of user input with fgets() or some version of your own as fgets() does have some weaknesses. Then validate the input.

char *input;
while ((input = my_gets()) != NULL) {
  if (valid_phrase(input)) {
    foo(input);
  } else {
    fprintf(stderr, "Invalid input\n");
  }
  free(input);
}

Concerning "I would love to get rid of the outer while loop". That loop exists to silently consume '\n'. If you want a loop to do that, just preceded inner loop with

int ch;
while ((ch = fgetc()) == '\n')
  ;
ungetc(ch, stdin);

char ch

The ch is not the best type. fgetc() returns typically 257 different values [0-255] and EOF. To properly distinguish them, save the result in an int.

// bad
char ch;
..
while (string == NULL && ch != EOF) {    
while (EOF != (ch = fgetc(in_stream)) && ch != '\n') {

// better
int ch;
..
while (string == NULL && ch != EOF) {
  while (EOF != (ch = fgetc(in_stream)) && ch != '\n') {

Same for char tmp;

realloc()

Cast not need.
Modify for out of memory to free string - not needed if code will simply exits, yet good practice to put your toys (code's pointer) away.

// string  = (char*) realloc(string, len+2);
char * new_string  = realloc(string, len+2);
if (new_string == NULL) {
  free(string);
  bail_out(EXIT_FAILURE, "Out of memory");
}
string = new_string;

Good use of of sizeof(*strings) below. Recommend simplification.

strings = (char**) realloc(strings, (index+1)*sizeof(*strings));
strings = realloc(strings, sizeof *strings * (index+1));

size_t len

Good use of size_t to represent an array size. Curiously code does not do the same with int index;. Recommend size_t index;

is...()

With using int ch, no need for cast. Since this is a logical test, recommend using ! rather than arithmetic == 0.

// if (ch != ' ' && isalpha((int)ch) == 0) {
if (ch != ' ' && !isalpha(ch)) {

The following may be easier to understand - fewer negations. (Style issue)

if (!(ch == ' ' || isalpha(ch))) {

ferror()

Nice check of if (ferror(in_stream))

Variable names

string, strings is as useful as calling an integer integer. Maybe phrase, dictionary instead.

// OK
/** @brief Contains the dictionary */
static char **strings = NULL;

// Better
// comment not truly needed
static char **dictionary = NULL;

get_strings() is mis-named. It sounds general, yet code restricts input to letters and space. Maybe get_words()?

Answer 2

Bad realloc strategy

Currently, you call realloc() on every character you read. This results in an \$O(n^2)\$ time to read a string, because every time you call realloc(), it may need to copy the current contents to the new buffer. You should either just allocate a buffer of size MAX_DATA and then use realloc to shrink the allocation at the end, or change to a reallocation strategy where the reallocation size is increased by a multiplicative factor each time (such as 2x).

This also applies to your array of strings, where you do the same thing.

Strange indentation

Your indentation is strange because your nested while loop is on the same indentation level as the outer while loop.

Use fgets()?

I would personally use fgets() (or some other library function such as readline()) to read a string. fgets() does pretty much what your loop does without all the handcoded logic.