Processing student scores from the stdin

Question

As part of learning more C, I wrote this extremely trivial C program that takes Student scores from stdin and, upon termination, prints out the minimum score/maximum score and the average score. Can somebody review my code and tell me how I might be able to improve my code?

#include <stdio.h>
int main()
{
 int currentEntry;
 int startValue = 0;
 int counter = 0;
 int currentMax;
 int currentMin;
 double average;
 printf("Enter scores one-by-one, exit by entering -1\n");
 while(currentEntry != -1){
 if((scanf("%d",&currentEntry) == 1) && (currentEntry != -1)){
 printf("Current score is %d\n",currentEntry);
 counter++;
 startValue += currentEntry;
 average = startValue/counter;
 if(counter == 1){
 currentMax = currentEntry;
 currentMin = currentEntry;
 }
 if(currentEntry > currentMax){
 currentMax = currentEntry;
 }
 if(currentEntry < currentMin){
 currentMin = currentEntry;
 }
 }
 }
 printf("The average is %lf\n",average);
 printf("The maximum score is %d\n",currentMax);
 printf("The minimum score is %d\n",currentMin);
 return 0;
}

Answer 1

You should get into the habit of initialising variables whenever possible. It's a little more difficult here, since currentMax, currentMin, and currentEntry may seem not to have a sensible default value. However, all arithmetic values have an upper and a lower bound (these can be found in limits.h). It would make sense to initialise currentMax and currentMin based on these:

int currentMax = INT_MIN;
int currentMin = INT_MAX;

Note that if the user inputs -1 straight away, currentMax and currentMin will now print out predictable values. If you leave them uninitialised, they will print out whatever bits happen to be on the stack at that point in time (effectively, they will print out a random value).

Also, note that because these are now initialised with the min/max integer values respectively, you can remove the check for counter being 1.

I'd also initialise currentEntry and average, 0 and 0.0 seem like relatively reasonable defaults.

The name startValue is accurate at the beginning, but by the end, doesn't really reflect the starting value as you've been adding to it each time. Hence, I'd rather call it something like currentTotal.

Integer division in C can trip beginners up, the line:

average = startValue/counter;

isn't doing quite what you want it to be. Since startValue and counter are both integers, this will do an integer division, and then convert that to a double. Hence this will actually be the floor of the average. To fix this, you need to cast the numerator or denominator to a double:

average = (double)startValue / counter;

In the end, the code is pretty similar, but looks something like:

#include <stdio.h>
#include <limits.h>
int main()
{
 int currentEntry = 0;
 int currentTotal = 0;
 int counter = 0;
 int currentMax = INT_MIN;
 int currentMin = INT_MAX;
 double average = 0.0;
 printf("Enter scores one-by-one, exit by entering -1\n");
 while(currentEntry != -1) {
 if((scanf("%d", &currentEntry) == 1) && (currentEntry != -1)) {
 printf("Current score is %d\n", currentEntry);
 counter++;
 currentTotal += currentEntry;
 average = (double)currentTotal/counter;
 if(currentEntry > currentMax) {
 currentMax = currentEntry;
 }
 if(currentEntry < currentMin) {
 currentMin = currentEntry;
 }
 }
 }
 printf("The average is %lf\n",average);
 printf("The maximum score is %d\n", currentMax);
 printf("The minimum score is %d\n", currentMin);
 return 0;
}

• sc_ray: Thanks a lot. I feel like I am learning more from Code Review than from any of the C books that are on my shelf.

Answer 2

Adding to what @Yuushi said, I would make some more changes.

Variable Names

Your variable names mostly contain the word 'current'. Does that improve readability or make it more understandable? I think not, so I would rename the variables total, count, min, max and average. currentEntry holds a 'score' so I would call it score. And counter can be shortened to count with no loss. Once you change these names in your code it will immediately look less dense and more readable.

Note that variable name length is often best if related to variable scope. So a variable that has large scope (ie. is used in many places) should be relatively long; ones that have small scope (and that is often most) may be short.

User input

Using scanf can cause problems. For example if you type a letter instead of a number your program will fail to recognize any subsequent input (the letter remains in the input stream and scanf (which was expecting a number) will not remove it. I hardly ever use scanf.

I would rewrite your loop to use fgets instead. This function reads a string from the input. You can then extract the score from the string using library function strtol. When you do this you can allow the user to type (for example) 'q' to quit instead of the somewhat contrived -1.

Here is the code:

int main(void)
{
 int total = 0;
 int count = 0;
 int max = INT_MIN;
 int min = INT_MAX;
 char buf[20];
 printf("Enter scores one-by-one, exit by entering 'q'\n");
 while (fgets(buf, sizeof buf, stdin) != NULL) {
 if (buf[0] == 'q') {
 break;
 }
 char *end;
 int score = (int) strtol(buf, &end, 0);
 if (end != buf) {
 count++;
 total += score;
 if (score > max) {
 max = score;
 }
 if (score < min) {
 min = score;
 }
 }
 }
 if (count) {
 double average = (double)total/count;
 printf("The average is %lf\n",average);
 printf("The maximum score is %d\n", max);
 printf("The minimum score is %d\n", min);
 }
 return 0;
}

Notice that the loop now exist when fgets returns NULL, which it does when the user closes the input stream (for example by typing ctrl-d on UNIXy systems). And there is an explict check for 'q' in the input.

The strtol call takes the buffer holding user input and also the address of a pointer &end. It places a pointer to the first character that is not part of a number in end. So if the user entered '123p', end would point to the 'p'. Also if buf does not contain a number at the beginning, strtol sets end to point to buf. Hence we can check whether a number was read by checking that equivalence.

Finally note that the average need not be computed on every loop so I have moved that outside the loop, protected by a check for count being non-zero.