1

So I have this piece of code which is written for reading the user input which is supposed to be between the numbers 1-9. In the code there is and int "choice" declared. But instead of directly scanning the user input using scanf("%d", choice); the programmer has used a char buffer and scanned the buffer instead and then used the atoi function to convert the char input to an integer. I'm confused why has it been done like that instead of directly doing it the easy way. My assumption is that the programmer has done this so that if the user enters a character instead of a number, the code doesn't malfunction. But if that's the case then how would the atoi convert an alphabet to an integer? Here's the code:

int readMenuChoice() {
    while (1) {
        char buffer[50];
        size_t buffLen =  10;
        int choice;
        showMenu(); //another function that displays all options from 1 to 9
        printf("Choose a menu option: ");
        scanf("%[^\n]", buffer);
        getchar();
        choice = atoi(buffer);
        if (choice > 0 && choice < 9) {
            return choice;
        }
        printf("Invalid input\n\n");
    }
}
klutt
  • 30,332
  • 17
  • 55
  • 95
Abi
  • 13
  • 3
  • "_how would the atoi convert an alphabet to an integer_" - It wouldn't - and the condition to exit the `while` loop (`if(choice > 0 && choice < 9)`) would not be met so it'd `showMenu()` again and let the user try to enter a valid choice again. – Ted Lyngmo Jul 06 '21 at 08:59
  • 1
    I would not do it like that. The author also has `size_t buffLen = 10;` which is irrelevant. It also has potential buffer overflow. It doesn't allow choice 9 as invited by the commented menu call. Very poor coding, an example of how *not* to code. – Weather Vane Jul 06 '21 at 09:11
  • 2
    Never use `atoi()`. It has absolutely no way to detect any errors or bad input. [**Why shouldn't I use atoi()?**](https://stackoverflow.com/questions/17710018/why-shouldnt-i-use-atoi) If the user types "Fish!" in response to your "Choose a menu option:" question, `atoi()` returns an integer that's valid. – Andrew Henle Jul 06 '21 at 09:15
  • @WeatherVane So should I be directly using the scanf and read the integer instead of doing all this? Or is there another way to go ahead with this? – Abi Jul 06 '21 at 13:28
  • There are many ways to do it. You get a lot of control, flow and checking options by using `fgets` to input and then `strtol` to convert. This [Get safe int input](https://stackoverflow.com/questions/53437829/c-get-safe-int-input) is one of several previous questions that address the topic. – Weather Vane Jul 06 '21 at 13:32

2 Answers2

1

We can only guess intention from the coder.

But a pretty likely reason is to make sure that the input stream is empty between each input. I personally do things like that too. But I would have done like this instead:

while (1) {
    char buffer[50];
    int choice;

    showMenu(); 
    printf("Choose a menu option: ");

    if(!fgets(buffer, sizeof buffer, stdin)) {
        /* Handle error */
    }

    if(sscanf(buffer, "%d", &choice) != 1) {
        /* Handle error */
    }

    if (choice > 0 && choice < 9) {
        return choice;
    }

    printf("Invalid input\n\n");
}

atoi is an unsafe function. If the argument cannot be parsed to a number, it invokes undefined behavior. And since x = atoi(s) is completely equivalent to sscanf(s, "%d", &x), there's no reason to use the unsafe function. sscanf returns the number of successful assignments, so it CAN be error checked.

klutt
  • 30,332
  • 17
  • 55
  • 95
  • *It returns zero on zero and error.* It's even worse than that. `atoi()` can return anything on errors - [If the value of the result cannot be represented, the behavior is undefined.](https://port70.net/~nsz/c/c11/n1570.html#7.22.1p1) – Andrew Henle Jul 08 '21 at 11:29
  • @AndrewHenle True. Fixed. – klutt Jul 08 '21 at 12:37
1

The problem of reading user input safely, limiting the input to a certain set of "allowed" inputs, while cleanly disregarding "disallowed" inputs, can be a surprisingly tricky one.

Also surprising, perhaps, is how poor a function scanf is for performing this task, and how difficult it is to solve the problem completely using any algorithm built around scanf.

You asked why this code didn't "directly do it the easy way". By "the easy way" I assume you mean something like

scanf("%d", &choice);

The problem here is that, yes, it can be remarkably difficult to proceed correctly if the user types some non-numeric input.

There are two general avenues to take when trying to handle the possibility that the user types something wrong:

  1. Continue to call scanf("%d") to read the input, but then, try to patch things up if scanf fails. (Obviously the first step here is to check scanf's return value.)
  2. Read a line of input, as text, using something other than scanf. Then attempt to validate that line, and convert it to the desired form.

In my opinion, there is only one choice here, and it is #2. This answer will become far too long if I discuss all the reasons, but the bottom line is that approach #1 is futile. The scanf function has one virtue and one virtue only, and that is that a call like scanf("%d", &choice) is indeed very simple. But the error handling is almost useless. By the time you've built up a reasonable amount of error handling around it, the amount of work you'll have had to do is about three times as much as for approach #2, and you still won't have fully satisfactory results.

So most experienced C programmers will agree that #2 is the only viable approach in the long run. There's a central question advising on good ways of doing input using something other than scanf.

The problem with the code you've posted, IMO, is that it manages to combine the worst of both worlds. It does try to read a line of input as text, then process it later, but the way it reads that line of input is with... the dreaded scanf! And despite trying to be careful in several other ways, this code doesn't even check scanf's return value, so there are some classic problems (like premature EOF) that this code is still vulnerable to.

This code also contains a mysterious extra call to getchar, which is typical of scanf-using code, since stray newlines are almost always a problem.

This code also uses %[...], which is my least favorite scanf format. As I said, scanf's only virtue is simplicity, but a locution like "%[^\n]" is anything but simple. Yes, I know what it does, but IMO it completely defeats the purpose of using scanf for dirt-simple (if less than robust) user input.

But, yes, the primary intent of writing the code this way is probably "so that if the user enters a character instead of a number, the code doesn't malfunction". The code reads a line of text, as text, then attempts to convert the text to a number. You asked what the atoi function does with alphabetic input, and the answer is that (most of the time, anyway) it quietly returns 0. Since 0 isn't a valid input, this code will reject it, so in that sense it works.

To improve this function, the first thing to do would be to replace the calls to scanf and getchar with fgets. The next thing to do would be to replace atoi with strtol. And then it wouldn't be too bad.

Steve Summit
  • 45,437
  • 7
  • 70
  • 103
  • *" my least favorite scanf format"* - That says so much more than just the words :D – klutt Jul 08 '21 at 12:19
  • atoi is not required to return 0 on failure – klutt Jul 08 '21 at 12:21
  • @klutt Thanks for reminding me about `atoi`'s undefinedness, which is worse than I thought. (`atoi("x")` really ought to have defined behavior, but you're right, it doesn't.) Answer updated. – Steve Summit Jul 08 '21 at 12:33