Trying to reimplement GetInt()

Question

I want to reimplement the GetInt() function from the CS50 library. I've mostly succeeded with the following code, but if I enter a single character, or CTRL+, I'm left with an extra newline so that I have to press Return before I'm prompted to enter a number. I'm using an empty while getchar() loop to consume erroneous input, and this is the single bug I've found in this method.

Here is the code, note that I have constrained the number to be between 1 and 10 because I'm using it for mario.c and I will change that later.

int getint(void)
{
  int x;
  char key[3] = {0, 0, 0};
  char *ptr;
  char c;

  do
  {
    printf("Please enter a whole number between 1 and 10: ");
    fgets(key, 3, stdin);

    for (int i = 0; i < 2; i++)
    {
      if (key[i] != 0 && key[i] != 10)
      {
        if (!isdigit(key[i]))
        {
          key[0] = 0;
          key[1] = 0;
          key[2] = 0;
          while ((c = getchar()) != 10 && c != EOF) {}
          break;
        }
      }
    }

    x = strtol(key, &ptr, 10);

    if (x > 10)
      while ((c = getchar()) != 10 && c != EOF) {}
  }
  while (x < 1 || x > 10);

  return x;
}

I realize there are other problems with this code in terms of portability, but I want to solve this newline issue first.

Answer 1

Thanks to chad's advice, I removed the redundant for() loop, made my chars more readable, and changed x from an int to a long.

However, I couldn't quite figure out how to use the functions from errno.h, so after declaring strtol(), I did this:

//Check if only the return key was entered into the prompt;
//if that's true, set x to be too large for an int.
//This prevents the function from returning 0,
//unless the user explicitly entered it.

if (key[0] == '\n')
  x = 2147483648;

//If the first char is not a newline, discard all chars up to the newline,
//then the newline itself.

if (!(c = strchr(key, '\n')))
{
  scanf("%*[^\n]");
  scanf("%*c");
}

Answer 2

Please use character constants such as '\n' and '\0' instead of opaque numbers like 10 and 0 respectively. Not only is it more portable, but it's much easier to read as well.

Anyway, your problem stems from the fact that fgets will read a line of input including the newline character if there is space for it in the buffer. When you enter in something like a, fgets will fill key with {'a','\n','\0'}. Since the newline has already been read in, your while loop will not exit until it has consumed another line of input.

Also, your code does not handle EOF appropriately. EOF means there is nothing more to be read. Currently, your code would just enter into an infinite loop upon reaching the end of a file, instead of returning some default value as it should.

You're also making your function much more difficult than it necessarily needs to be. strtol validates the string so your for-loop is not actually necessary. All you'd need to do is something like this:

#include <errno.h>
...
char string[] = "123";
char *endptr;
errno = 0;
long x = strtol(str, &endptr, 10);

// First non-numeric character was not the null byte 
// or the number was too large to fit in a long

if (*endptr != '\0' || errno == ERANGE) {
    // Error handling
}

Also, since a long has a wider range than an int you should make sure that x is between INT_MIN and INT_MAX.