2

writing a program that will be finding min, max, avg of values entered by user. Having trouble writing something that will check to make sure there are only postive integers entered and produce an error message. heres my for statement that is reading the input so far:

  for (int value = 0; value <= numofvals; ++value) {
      printf("Value %d: %f\n", value, val_input);
      scanf("%f", &val_input);
  }

mind you I've been learning code for about 3 weeks and was just introduced to loops this week so my understanding is rudimentary at best!

Schwern
  • 127,817
  • 21
  • 150
  • 290
Sterling
  • 23
  • 1
  • 4
  • How do you print a `val_input` that is read from `stdin` the line \*after\* your `printf()`? – Swordfish Oct 11 '18 at 23:32
  • 1
    You need to use an if-then statement after you read in the value to validate that the input conforms to whatever you (and more importantly, your program) expects. This is something that you'll end up spending lots of work doing in programming, since invalid input can be the cause of severe security issues and other bugs. Basic rule is to NEVER trust user input and always validate everything. – ivanivan Oct 11 '18 at 23:33
  • 1
    You can also check `if ((val_input >> (sizeof val_input * CHAR_BIT - 1)) & 1)` to check whether the *sign-bit* is set (bit 31 for single precision floating point and `int` or bit 63 for double precision floating point and 8-byte integer values) `CHAR_BIT` is the number of bits-per-character (generally 8) and is defined in `limits.h`. – David C. Rankin Oct 12 '18 at 01:53
  • What should happen if more than 1 number enter on a line? – chux - Reinstate Monica Oct 12 '18 at 13:49
  • What should happen with numeric input like `1.234` that is not an integer? – chux - Reinstate Monica Oct 12 '18 at 13:51
  • @chux ... confused ... since he is `scanf("%f", &val_input);`, that should work just fine? – David C. Rankin Oct 12 '18 at 14:23
  • @DavidC.Rankin OP's title is "accept only positive integer" yet uses a FP specifier. It appears post post-processing implied to insure value is a whole number. – chux - Reinstate Monica Oct 12 '18 at 14:58
  • It's just confused all the way around. But yes, I get your point, unless he handles the offending chars on a matching failure, the euphemisms of "spinning your wheels", "chasing your tail", or "activity without accomplishment" come to mind. – David C. Rankin Oct 12 '18 at 15:29

3 Answers3

2

First, don't use scanf. If stdin doesn't match what it expects it will leave it in the buffer and just keep rereading the same wrong input. It's very frustrating to debug.

const int max_values = 10;

for (int i = 0; i <= max_values; i++) {
    int value;
    if( scanf("%d", &value) == 1 ) {
        printf("Got %d\n", value);
    }
    else {
        fprintf(stderr, "I don't recognize that as a number.\n");
    }
}

Watch what happens when you feed it something that isn't a number. It just keeps trying to read the bad line over and over again.

$ ./test
1
Got 1
2
Got 2
3
Got 3
foo
I don't recognize that as a number.
I don't recognize that as a number.
I don't recognize that as a number.
I don't recognize that as a number.
I don't recognize that as a number.
I don't recognize that as a number.
I don't recognize that as a number.
I don't recognize that as a number.

Instead, use fgets to reliably read the whole line and sscanf to parse it. %f is for floats, decimal numbers. Use %d to recognize only integers. Then check if it's positive.

#include <stdio.h>

int main() {
    const size_t max_values = 10;
    int values[max_values];
    char buf[1024];

    size_t i = 0;
    while(
        // Keep reading until we have enough values.
        (i < max_values) &&
        // Read the line, but stop if there's no more input.
        (fgets(buf, sizeof(buf), stdin) != NULL)
    ) {
        int value;

        // Parse the line as an integer.
        // If it doesn't parse, tell the user and skip to the next line.
        if( sscanf(buf, "%d", &value) != 1 ) {
            fprintf(stderr, "I don't recognize that as a number.\n");
            continue;
        }

        // Check if it's a positive integer.
        // If it isn't, tell the user and skip to the next line.
        if( value < 0 ) {
            fprintf(stderr, "Only positive integers, please.\n");
            continue;
        }

        // We got this far, it must be a positive integer!
        // Assign it and increment our position in the array.
        values[i] = value;
        i++;
    }

    // Print the array.
    for( i = 0; i < max_values; i++ ) {
        printf("%d\n", values[i]);
    }
}

Note that because the user might input bad values we can't use a simple for loop. Instead we loop until either we've read enough valid values, or there's no more input.

Schwern
  • 127,817
  • 21
  • 150
  • 290
  • `scanf` is perfectly fine. Not all input is "interactive user input", as that linked FAQ seems to base its answer off. Also, you don't need to do line-buffering (with its added problems). Instead, if `scanf` returns `0`, you can simply skip characters until, for instance, the first whitespace character. – Acorn Oct 12 '18 at 00:36
  • 1
    @Acorn: If you aren't dealing with interactive input, you don't need prompts. – Jonathan Leffler Oct 12 '18 at 00:40
  • 3
    @Acorn [We get so many questions about `scanf`](https://stackoverflow.com/search?q=scanf+%5Bc%5D) that boil down to "scanf left stuff on the buffer" and "check the return value". So many. So very many. – Schwern Oct 12 '18 at 00:40
  • @JonathanLeffler: Yeah, but I was talking about the FAQ (not this particular question). Anyway, even if there are prompts, `scanf` is typically perfectly fine as well. If you are trying to parse complex lines from the user (e.g. commands), then sure, use something else. But for many cases, and in particular this question (OP is a beginner), is the proper approach. OP possibly does not even know how to use arrays. – Acorn Oct 12 '18 at 00:43
  • @Schwern That is a popularity argument, nothing else :) – Acorn Oct 12 '18 at 00:45
  • 2
    @Acorn It's an indication of how often people get caught by `scanf`'s gotchas. – Schwern Oct 12 '18 at 02:19
  • 3
    @Acorn -- "don't use `scanf()`" is very good advice, _especially for learners_. This function is difficult to use correctly, and has so many dark corners that even those who know what they are doing can get caught by its quirks. There is almost always a better solution than `scanf()`. – ad absurdum Oct 12 '18 at 04:55
  • @Schwern: That does not prove anything. More people may be getting caught by non-`scanf` approaches, but it may not be as popular. – Acorn Oct 12 '18 at 10:06
  • @DavidBowling: Sorry, no. `scanf` being complex/featureful does not mean it cannot be simple to use for simple case --- which it is, and it is why it is typically the suggested way; see e.g. https://stackoverflow.com/questions/5087062/how-to-get-int-from-stdio-in-c. Now, please tell me what are those "better solutions" than `scanf` than can be *easily be applied by a learner* and that are in the standard C library. – Acorn Oct 12 '18 at 10:19
  • 1
    @Acorn -- `fgets()` followed by `sscanf()` is very simple to use, almost as simple as naked `scanf()`, more flexible, easier to use while avoiding problems with the input stream, and better to teach learners in my opinion. – ad absurdum Oct 12 '18 at 13:04
  • @DavidBowling That is neither easier (two function calls + requires buffer) nor more flexible (requires a buffer). – Acorn Oct 12 '18 at 13:07
  • 2
    @Acorn -- it is easier, and more flexible. Using `fgets()` followed by `sscanf()` means that lines of input are fetched and stored in a generously sized buffer which can be scanned again when input does not match initial expectations. The ability to rescan input makes it more flexible. And unless the input was too large, `stdin` will be empty after the call to `fgets()` (hence the generously sized buffer), meaning that there is no need for the code that cleans the input stream. – ad absurdum Oct 12 '18 at 13:10
  • 1
    @Acorn Two simpler function calls for two operations (reading and parsing) is simpler than one more complex function call for two operations. Each individual step is simpler to understand. It is explicit what was read in and what the state of the file pointer is. The line can be reused or parsed elsewhere. `scanf` is complex and less flexible because it crams multiple operations together, reading and parsing. And the parsing rules are very complex. And it isn't clear in what state it leaves `stdin`. And all that is opaque. Thank you for your answer showing how `scanf` can be used. – Schwern Oct 12 '18 at 14:35
  • @DavidBowling It is harder, and less flexible. Using `fgets()` followed by `sscanf()` only achieves using extra memory, increases complexity and cannot deal with arbitrarily long lines. There is no need **whatsoever** to rescan the input for this problem. – Acorn Oct 12 '18 at 23:53
  • @Schwern `sscanf()` is as complex as `scanf()` (to the point that both are specified in terms of `fscanf()`). Therefore, `fgets()` + `sscanf()` cannot be simpler than a single `scanf()`. Further, what do you find "opaque" about `scanf()` or the state of `stdin`? – Acorn Oct 13 '18 at 00:01
  • 1
    @Acorn First let me say that it's great that you understand `scanf` so well, and it's very powerful *after you spend time studying it*. `fgets` + `sscanf` can ignore the most bewildering parts of `scanf`: quietly rewinding the buffer and its rules for consuming a newline, as evidenced by all the questions and FAQs about those behaviors. And there's no need to undo the rewind. By using `fgets` we know we will always advance to the next line. `sscanf` here is a shorthand for "convert the string to an integer and ignore the newline". It also allows separating reading lines from a parsing them. – Schwern Oct 13 '18 at 00:49
  • 1
    @Acorn Because `fgets` + `sscanf` separates reading from parsing I can [move all parsing into a function](https://gist.github.com/schwern/c3b81eae84f9c373e45829b1adb242e7) simplifying the read loop and allowing the parsing function to be reused and unit tested. Now that the parsing code is isolated I can realize [I don't need `sscanf` and use `strtol` instead](https://gist.github.com/schwern/6a9d3b0a7a6c07db94588e88ddfceaa4) without effecting any code outside the parsing function. With `scanf` this is more difficult because reading and parsing are welded together. – Schwern Oct 13 '18 at 01:05
  • @Schwern Non sequitur: you can also make a function called `read_positive_int` that reads from `stdin` and such function can also be reused and unit tested equally well (actually, writing such a function is a very common exercise). When you are working with streams, reading and parsing "at the same time" is *precisely* what you want to do. – Acorn Oct 13 '18 at 01:24
  • @Acorn Yes, sometimes you need to be stream oriented. This problem is line oriented. – Schwern Oct 13 '18 at 02:36
  • @Schwern No, sorry. There is nothing line-oriented about the problem stated by OP. How can you say so? You are puzzling me. – Acorn Oct 13 '18 at 07:26
  • @Acorn You are technically correct, the OP doesn't say line or stream, and they probably don't even know. I went with line-by-line for simplicity. Yours parses by stream but sometimes invalidates by line, sometimes by item. Its behavior is inconsistent. `1 2 3\n` reads as 1, 2, and 3. But `1 foo 3\n` reads as 1 and "not an integer". The 3 is quietly ignored. `1 -2 3\n` reads as 1, "not a positive integer", and 3. I'd be interested to see how that inconsistency can be corrected. – Schwern Oct 13 '18 at 18:16
1

Something easy like this may work for you:

int n;
int ret;

for (;;) {
    ret = scanf("%d", &n);

    if (ret == EOF)
        break;

    if (ret != 1) {
        puts("Not an integer");
        for (;;)
            if (getchar() == '\n')
                break;
        continue;
    }

    if (n < 0) {
        puts("Not a positive integer");
        continue;
    }

    printf("Correct value %d\n", n);

    /* Do your min/max/avg calculation */
}

/* Print your results here */

This is just an example and assumes you do not need to read floating point numbers and then check if they are integers, as well as a few other things. But for starters, it is simple and you can work on top of it.

To break out of the loop, you need to pass EOF (typically Ctrl+D in Linux/macOS terminals, Ctrl+Z in Windows ones).

Acorn
  • 22,093
  • 4
  • 30
  • 62
-2

An easy and portable solution

#include <limits.h>
#include <stdio.h>
int get_positive_number() {
  char buff[1024];
  int value, ch;
  while (1) {
    printf("Enter positive number: ");
    if (fgets(buff, 1023, stdin) == NULL) {
      printf("Incorrect Input\n");
      // Portable way to empty input buffer
      while ((ch = getchar()) != '\n' && ch != EOF)
        ;
      continue;
    }
    if (sscanf(buff, "%d", &value) != 1 || value < 0) {
      printf("Please enter a valid input\n");
    } else {
      break;
    }
  }
  return value;
}
void solution() {
  // Handling malformed input
  // Memory Efficient (without using array to store values)
  int n;
  int min = INT_MAX;
  int max = INT_MIN;
  double avg = 0;
  printf("Enter number of elements: ");
  scanf("%d", &n);
  getc(stdin);
  int value;
  for (int i = 0; i < n; i++) {
    value = get_positive_number();
    if (value > 0) {
      if (min > value) {
        min = value;
      }
      if (max < value) {
        max = value;
      }
      avg += value;
    }
  }
  avg = avg / n;
  printf("Min = %d\nMax = %d\nAverage = %lf\n", min, max, avg);
}
int main() {
  solution();
  return 0;
}
  • Output:


Enter number of elements: 3
Enter positive number: 1
Enter positive number: 2
Enter positive number: a
Please enter a valid input
Enter positive number: -1
Please enter a valid input
Enter positive number: 1
Min = 1
Max = 2
Average = 1.333333
shirish
  • 673
  • 4
  • 8
  • 4
    Not my downvote, but…the `fseek(stdin, 0, SEEK_END);` line is highly debatable. Many times, `stdin` is not a seekable device. Skipping the entire input file because of an error in one line may be too draconian. (See also [Using `fflush(stdin)`](https://stackoverflow.com/questions/2979209/using-fflushstdin) for more spin on this topic.) I'd also prefer to see `printf("%d\n", n);` with the newline at the end of the format string. – Jonathan Leffler Oct 12 '18 at 00:31
  • 1
    This does not print any error message nor reads the values for processing, simply searches for the first correct one. – Acorn Oct 12 '18 at 00:32
  • You're really better off using `fgets` and `sscanf` then trying to patch up `scanf`. – Schwern Oct 12 '18 at 00:36
  • 1
    Note that the new code ("an easy way") runs into problems if the user only indicates EOF (`n` is used uninitialized), or if the user types a letter (or other non-numeric characters) instead of a number. You must check the return value from `scanf()`. If `scanf()` returns EOF, you should probably abandon the loop; if it returns `0`, you should probably think about reading characters up to the next newline, or maybe until the next character that could be numeric — a `+`, `-` or a digit when reading integers. Robust input is depressingly hard. – Jonathan Leffler Oct 12 '18 at 00:39
  • @shirish Please read the question carefully. This is not about accepting the first valid positive integer value (i.e. this will likely confuse OP). – Acorn Oct 12 '18 at 00:49
  • @Acorn please review – shirish Oct 12 '18 at 04:58
  • 2
    This code has undefined behavior when the input is malformed, e.g., if the user enters letters instead of numbers. – ad absurdum Oct 12 '18 at 05:14
  • @shirish: There is no need to keep all the values in memory to compute the minimum, maximum and/or average; therefore, you can simplify quite a bit! :-) – Acorn Oct 12 '18 at 10:19
  • `while (scanf("%d", &value) != 1 || value < 0)` is an infinite loop on end-of-file. – chux - Reinstate Monica Oct 12 '18 at 13:43
  • No need for -1 in `char buff[1024]; ... fgets(buff, 1023, stdin);`, `fgets(buff, sizeof buf, stdin);` is better. – chux - Reinstate Monica Oct 12 '18 at 13:45
  • `sscanf(buff, "%d", &value)` does not insure the previous `fgets(buff,...` succeeded. – chux - Reinstate Monica Oct 12 '18 at 13:46
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/181769/discussion-between-shirish-and-chux). – shirish Oct 12 '18 at 16:11