patterncMinor
Processing student scores from the stdin
Viewed 0 times
processingstudentthestdinscoresfrom
Problem
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
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",¤tEntry) == 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;
}Solution
You should get into the habit of initialising variables whenever possible. It's a little more difficult here, since
Note that if the user inputs
Also, note that because these are now initialised with the min/max integer values respectively, you can remove the check for
I'd also initialise
The name
Integer division in
isn't doing quite what you want it to be. Since
In the end, the code is pretty similar, but looks something like:
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
#include
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", ¤tEntry) == 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;
}Code Snippets
int currentMax = INT_MIN;
int currentMin = INT_MAX;average = startValue/counter;average = (double)startValue / counter;#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", ¤tEntry) == 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;
}Context
StackExchange Code Review Q#39785, answer score: 2
Revisions (0)
No revisions yet.