0

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.

0

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");
}
1
  • x = 2147483648 will overflow when you return it as an int which is undefined behavior. CS50's get_int returns INT_MAX. True, this is indistinguishable from the user actually entering INT_MAX (2147483647), but that's the best you can do without introducing some other way to return an error.
    – chad
    Apr 30 '17 at 22:49
0

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.

6
  • I've never used errno.h before. How do I declare ERANGE? Apr 29 '17 at 15:09
  • Also, why a char* and not a char array? I'm trying to do it that way, but I segfault when fgets tries to store the value from STDIN in the char*. Apr 29 '17 at 21:36
  • You don't declare ERANGE or errno, they are part of errno.h. Run man errno for more information on what it is. With regards to the char* instead of char[], you are correct, my bad. String literals, unless declared as a char array, are immutable. I updated my answer.
    – chad
    Apr 30 '17 at 22:45
  • Thanks. I checked out the man page, and it says that errno will never be zero. Doesn't strtol return 0 on an error, though? That was what I had to work around by intentionally setting x to a number outside INT_MAX. This is intriguing, but I'm having a hard time wrapping my head around it. May 17 '17 at 3:00
  • You can look at the actual library to see how it works at github.com/cs50/libcs50
    – chad
    Jun 1 '17 at 3:43

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .