Have to hit enter twice with fgets() in C?

Question

Good Morning, I'm having an issue with some C code where I am forced to hit enter twice each time input is entered if the length of the input is less than the size of 'guess'.

If the length of the input is longer than guess, I only hit enter once, and it functions normally.

I'm not sure what the issue is here, but I provided the function in question that I believe is the source of the problem along with the caller function and main just for context.

Output:

Guess a number: 5555555555
Invalid guess.
Guess a number: 55555555555
Invalid guess.
Guess a number: 555

Invalid guess.
Guess a number: 

Invalid guess.
Guess a number: 5555

When I remove the line:

while((ch = getchar()) != '\n' && ch != EOF); // Flush the input buffer

and I extend past the size of the buffer, I receive this output:

Guess a number: 5555555555555555555555555555555555
Invalid guess.
Guess a number: Invalid guess.
Guess a number: Invalid guess.
Guess a number: Invalid guess.
Guess a number: Invalid guess.

Function in Question

char * get_input(char * guess)
{
    print_message("guess"); // Prompt user to input a guess
    fgets(guess, sizeof (guess), stdin);
    if(feof(stdin))
    {
        printf("error");
        exit(EXIT_FAILURE);
    }
    int ch = 0;
    while((ch = getchar()) != '\n' && ch != EOF); // Flush the input buffer
    guess[strlen(guess)-1] = '\0'; // Erase new line character
    return guess;
}

Caller Function

int make_guess(int *v_guess_count)
{
    int result = 0;
    bool valid = false;
    char guess[10] = {'\0'}; // Buffer to store the guess
    do
    {
        get_input(guess); // Get the input
        if(is_valid(guess)) // Check if the input is valid
        {
            valid = true;
            *v_guess_count += 1;
        }
    }
    while (! valid); // Keep asking for input until guess is valid
    result = assign_value(guess); // Assign the guess
    return result;
}

Main

int main(int argc, char * argv[])
{
    int red = 0;
    int white = 0;
    int v_guess_count = 0;
    int target = get_target();
    bool game_won = false;
    while(game_won == false)
    {
        red, white = 0; // Reset to zero after each guess
        int guess = make_guess(&v_guess_count); // Make a guess. If it's valid, assign it.
        printf("guess is: %d\n", guess);
        compare(guess, target, &red, &white); // Check the guess with the target number.
        print_hints(&red, &white);
        if (red == 4)
        {
            game_won = true;
        }
    }
    printf("You win! It took you %d guesses.\n", v_guess_count);
    return 0;
}

Answer 1

You have two somewhat-related problems.

One. In your function

char * get_input(char * guess)

your line

fgets(guess, sizeof (guess), stdin);

does not do what you think it does. You want to tell fgets how big the buffer is, that is, how much memory is pointed to by guess for fgets to read into. But in function get_input, parameter guess is a pointer, so sizeof(guess) is going to be the size of that pointer, not the size of the array it points to. That is, you're going to get a size of probably 4 or 8, not the 10 that array guess up in make_guess is declared as.

To fix this, change your input function to

char * get_input(char * guess, int guess_size)

and change the call in make_guess to

get_input(guess, sizeof(guess));

For more on this point, see this question and also this answer.

Two. Your array guess for reading the user's guess is too small. Instead of making it size 10, make it size 500 or something. That way it will "never" overflow. Don't worry that you're wasting memory by doing that — memory is cheap.

The reason for making the input buffer huge is this: If you make the buffer small, you have to worry about the case that the user might type a too-long line and that fgets might not be able to read all of it. If you make the buffer huge, on the other hand, you can declare that the problem "won't happen" and that you therefore don't have to worry about it. And the reason you'd like to not worry about it it is that worrying about it is hard, and leads to problems like the one you've had here.

To use fgets strictly correctly, while worrying about the possibility that the user's input overflows the buffer, means detecting that it happened. If fgets didn't read all the input, that means it's still sitting there on the input stream, waiting to confuse the rest of your program. In that case, yes, you have to read or "flush" or discard it. That's what your line

while((ch = getchar()) != '\n' && ch != EOF);

tries to do — but the point is that you need to do that only if fgets had the not-big-enough problem. If fgets didn't have the problem — if the buffer was big enough — you don't want to do the flush-the-input thing, because it'll gobble up the user's next line of intended input instead, as you've discovered.

Now, with this said, I have to caution you. In general, a strategy of "make your arrays huge, so you don't have to worry about the possibility that they're not big enough" is not a good strategy. In the general case, that strategy leads to insecure programs and horrible security problems due to buffer overruns.

In this case, though, the problem isn't too bad. fgets is going to do its best not to write more into the destination array than the destination array can hold. (fgets will do a perfect job of this — a perfect job of avoiding buffer overflow — if you pass the size correctly, that is, if you fix problem one.) If the buffer isn't big enough, the worst problem that will happen is that the too-long part of the input line will stay on the input stream and get read buy a later input function, thus confusing things.

So you do always want to think about the exceptional cases, and think about what your program is going to do under all circumstances, not just the "good" ones. And for "real" programs, you do have to strive to make the behavior correct for all cases. For a beginning program like this one, though, I think most people would agree that it's fine to just use a huge buffer, and be done with it.

If you want to go for the extra credit, and perfectly handle the case where the user typed more than the fgets input buffer will hold, you're going to first have to detect that case. The code would look something like:

if(fgets(guess, guess_size, stdin) == NULL)
{
    printf("error");
    exit(EXIT_FAILURE);
}

if(guess[strlen(guess)-1] != '\n')
{
    /* buffer wasn't big enough */
    int ch = 0;
    while((ch = getchar()) != '\n' && ch != EOF); // Flush the input buffer
    /* now print an error message or something, */
    /* and ask the user to try again with shorter input */
}

But the point is that you do the while((ch = getchar()) != '\n' && ch != EOF) thing only in the case where fgets failed to read the whole line, not in the case where it succeeded.

---

If you're still with me, here are two somewhat-important footnotes.

1. I suggested changing your get_input function to take a second parameter int guess_size, but it turns out a better type to use for the sizes of things is size_t, so a better declaration would be size_t guess_size.

2. I suggested the test if(guess[strlen(guess)-1] != '\n') to detect that fgets wasn't able to read a full line, but that could fail (pretty badly) in the obscure case where fgets somehow returned an empty line. In that case strlen(guess) would be 0, and we'd end up accessing guess[-1] to see if it was the newline character, which is undefined and wrong. In practice it's probably impossible for fgets to return an empty string (at least, as long as you give it a buffer bigger than 1 to read into), but it's probably easier to write the code in a safer way than to convince yourself that it can't happen. There are a bunch of questions elsewhere on SO about practically and efficiently detecting the case that fgets didn't read a full line successfully, but just now I can't find any of them.