Title: Student Code Critique Competition 32

#include <iostream>
#include <iomanip>
using namespace std;

int main() {
int test1, test2, test3, test4,test5,average,
                          averagescore,divide;
cout <<"This program will gather five test
                                scores and\n";
cout <<"drop the lowest score, giving you the
                                   average\n";
cout <<"\n";
cout <<"Please enter Five Test scores\n";
cin >> test1>>test2>>test3>>test4>>test5;
int lowest = test1;
       // test 1 is the lowest number unless
if (test2 < test1)lowest = test2;
       // test 2 is smaller than test 1 unless
if (test3 < test2)lowest = test3;
       // test 3 is smaller than test 2 unless
if (test4 < test3)lowest = test4;
       // test 4 is smaller than test 3 unless
if (test5 < test4)lowest = test5;
       // test 5 is smaller than test 4.
average = (test1+test2+test3+test4+test5);
       // all test scores averaged together
averagescore = average - lowest;
       // average score minus the lowest grade
divide = averagescore /4;
       // average grade is then divided by 4
cout << divide<< endl;
       // final grade after division
return 0;
}

I do have to admit that on first scan of the code, I didn't notice the error. It was only when typing the code in that I noticed it.

One thing that I often do when working with numbers is to actually transpose the numbers into the code and look for errors. It is very easy to get caught by "off by one" errors, however this is not one of those times. Firstly let's look at the first sequence of numbers:

80 84 60 100 90

Now put these into the code replacing the test variables (let's replace the comments too):

int lowest = 80;
// 80 is the lowest number unless
if (84 < 80) lowest = 84;
// 84 is smaller than 80 unless
if (60 < 84) lowest = 60;
// 60 is smaller than 84 unless
if (100 < 60) lowest = 100;
// 100 is smaller than 60 unless
if (90 < 100) lowest = 90;
// 90 is smaller than 100.

Now we can easily see that the logic is flawed. Checking the adjacent value will not choose the smallest. The simplest change that will get the code to work is the check against the current lowest value instead

int lowest = test1;
         // test 1 is the lowest number unless
if (test2 < lowest) lowest = test2;
         // test 2 is smaller than lowest unless
if (test3 < lowest) lowest = test3;
         // test 3 is smaller than lowest unless
if (test4 < lowest) lowest = test4;
         // test 4 is smaller than lowest unless
if (test5 < lowest) lowest = test5;
         // test 5 is smaller than lowest.

Now to comment on style...

What is going to happen if we now need to test six values, or ten, or even a class of 30? The algorithm being used is not particularly extensible.

One solution is to calculate the sum and the lowest while entering value. However when doing this we now have to handle the boundary cases where the user may enter any number of values. No values obviously has average of zero, while one value is by definition also the lowest, and the average of the rest (no values) is zero, so the average calculation is only valid where the number of entered values is greater than one. Here is an example that accumulates on the fly:

#include <iostream>
#include <limits>
using namespace std;
int main() {
  cout << "This program will gather "
       << "test scores and drop the\n"
       << "lowest score, giving you the "
       << "average of the remaining.\n\n"
       << "Enter test scores.  Terminate "
       << "the last score with a period.\n";
  int sum = 0;
  int count = 0;
  int value;
  int lowest = numeric_limits<int>::max();
  while(cin >> value) {
    if(value < lowest) lowest = value;
    sum += value;
    ++count;
  }
  int average = 0;
  if(count == 0)
    cout << "No entries entered\n";
  else if(count == 1)
    cout << "Only one value entered, "
         << "so it is the lowest value.";
  else
    average = (sum - lowest) / (count - 1);
  cout << "Average: " << average << endl;
  return 0;
}

numeric_limits is used to define the initial value of the lowest variable. Since any other integer value will be equal or less than this, then any value typed in as the first value will set the lowest to be that. Subsequent values are then checked against the current lowest.

The other "trick" in the code is using the fail flag on cin to terminate the entry loop. The fail flag happens when we ask to stream into an integer and the stream contains a non-whitespace non-integer value, hence the terminating the last score with a period.

Another solution is to use standard containers and use algorithms like accumulate to sum the values, but this I'll leave as an exercise for the reader (or other submitters).

The code does not work because of a simple logical error, and not a language specific problem. Each if statement is evaluated "locally" and in effect ignores the preceding work done. Thus, as the student as observed, if test5 contains a smaller value than test4 then lowest is assigned test5, irrespective of the results of the preceding tests. The straightforward fix is to change the sequence of if statements to compare each time against the current lowest value, then I would expect the code to work.

This of course ignores issues of extensibility, design and style, but for a student of this level I would consider it more important to understand the logical flow required to solve the problem at this simple level. Ultimately knowledge of the standard library is second to a solid grasp of constructing a logical sequence of steps to solve a problem programatically. For future reading I would advise reading up on arrays and containers, and the std::sort algorithm. Constant use of std::endl vs \n would also be nice.

The first thing to do is to answer the student's question - they want to know what is wrong with the code. The answer is the sequence of comparisons of adjacent test values: the result of each stage (the new value of lowest) needs to be passed into the next comparison.

Simply change the sequence to:

int lowest = test1;
      // test 1 is the lowest number unless
if(test2 < lowest) lowest = test2;
      // test 2 is smaller than lowest so far unless
if(test3 < lowest) lowest = test3;
      // test 3 is smaller than lowest so far unless
if(test4 < lowest) lowest = test4;
      // test 4 is smaller than lowest so far unless
if(test5 < lowest) lowest = test5;
      // test 5 is smaller than lowest so far.

This fixes the code - but there are several other things worth commenting on. Firstly, this sort of code cries out for a loop! In order to do this we want an array variable rather than 5 separate variables. C++ comes with a suitable collection object: the vector. So we can replace the list of variables test1 to test5 with:

std::vector<int> test(5);

then the input, the test and the addition can all be done by using loops - this is immediately generalisable to cases where you've got more (or less) than 5 numbers to process.

To make the code more robust, this should be the last time we use the hard-coded number '5' - the rest of the program can use the size of the vector to ensure it copes with changes to this number.

for(int i = 0; i != test.size(); ++i)
  cin >> test[i];
int lowest = test[0];
for(int i2 = 1; i2 != test.size(); ++i2)
  if(test[i2] < lowest) lowest = test[i2];
          // get the lowest number
average = 0;
for(int i3 = 0; i3 != test.size(); ++i3)
  average += test[i3];

I'd also like to change the names of the variables - the names don't match the contents. For example, average and averagescore both contain totals, not averages!

It is good to pick meaningful names for variables, but important to remember to keep the names of the variables consistent with their usage to avoid leading the reader of the code astray.

Lastly, we can get rid of some of the loops by using algorithms provided by the standard library. We can use min_element to find the lowest value and accumulate to perform the sum.

My final version of the program looks like this:

#include <iostream>
#include <iomanip>
#include <vector>
#include <algorithm>
#include <numeric>
using namespace std;

int main() {
  cout << "This program will gather "
       << "five test scores and\n";
  cout << "drop the lowest score, "
       << "giving you the average\n" << "\n";
  cout << "Please enter Five Test scores\n";
  std::vector<int> test(5);
  for(int i = 0; i != test.size(); ++i)
    cin >> test[i];
  int lowest = *min_element(test.begin(),
                            test.end());
  int totalscore = std::accumulate(test.begin(),
                               test.end(), -lowest);
          // total score (minus the lowest grade)
  int divide = totalscore / ( test.size() - 1);
     // average grade is then total divided by (n-1)
  cout << divide << endl;
          // final grade after division
  return 0;
}

My hope is that the resultant code is easy to understand and does the task well enough. This code may be slower to write than the original code the first time, but with practice it will become second nature. The time spent learning to do this is also repaid in the reduction in bugs.

OK, so I am not a programmer but every now and then I get to look at code, especially when I want to know what actually happens in a program. (Especially when I do not believe the documentation or there is no documentation). Thank heavens this code compiles and runs - I don't think I could handle obscure syntax problems. Anyway, full marks to the student for carrying out sufficient testing to identify that there is a problem. Now, looking at the student's code a number of points to comment on jump out at me. I will treat them in order so the trivial are mixed in with the more important - but then with just a short piece of code it is not clear how a trivial comment would scale into a programme with thousands of lines of code.

1. Input validation

There is no validation of the input. This is not the student's issue but it is worth a remark as I would expect significant further programming effort to ensure that the input is properly validated (sensible means of stopping the programme if run in error, the correct number of exam grades are entered, each is a number, each within sensible bounds, meaningful error messages to the user, etc).

2. Choice of variable names

I do not like variable names which look like reserved words - average and divide (and further down lowest) make me uneasy. These names look as though they are self-documenting but I would prefer something like average_score and lowest_score as better self-documenting names. [See also comment 7 below].

3. Consistent programming style

I look for a consistent programming style as this should speed up comprehension of code. Here we have a block where all the int variables are defined except for the single variable lowest which is defined later. I may be making a mountain out of a molehill in this case but when a programme extends to thousands of lines then consistency is important.

4. Program logic

This is the guts of the student's problem. The process should be to set up a placeholder for the lowest grade. This is given any grade as its initial value (and the first grade is as good as any). Then each grade is compared in turn to the placeholder and if the grade has a lower value the placeholder is reset to that grade. From this description it follows that the if statements should read

if(testn < lowest)lowest = testn;

for each value of n from 1 to 5.

5. Generalisation (1)

I am against unnecessary generalisation but in this case I think it clarifies the program logic. If we allow for a variable number of tests we can generalise the logic into a for loop using an array of test scores. Something on the following lines should do

lowest_score = test_score(1);
for(int i = 1, i <= number_of_tests, i++) {
  if(test_score(i) < lowest_score)
    lowest_score = test_score(i);
}

(The student might want to change the input process to start with obtaining the number of grades, so giving a value to the new variable number_of_tests - there are other approaches which could be used).

A bit of cleverness might be to avoid the first iteration in the loop as this is unnecessary. But that, in my opinion, tends to obscure the logic process.

6. Comments should be meaningful (1)

I found the comments against all the if statements were not very helpful. In fact one reason for generalising to the for loop above was to think how the for loop should be commented and compare this to the existing comments. In fact I would not bother to comment the for loop at all.

7. Variable names are misleading

The variable average is set to a total and is not an average at all. As variable names, average and averagescore give no clues as to the different (or same) data entities they refer to. divide gives no clues at all to what it means. I would suggest that the following are more meaningful values

total_score for average

adjusted_total_score for averagescore

final_grade for divide

8. Comments should be meaningful (2)

The comments in the section of code which calculates the final grade are wrong. (Note the distinction - the variable names are misleading, the comments are wrong). With good variable names (such as those suggested above) I think that comments are unnecessary.

9. Magic numbers

The evaluation of divide uses the "magic number" 4. This comment is scarcely worth bothering about in this specific example but is something to be aware of if the programme is generalised to handle any number of grade scores.

10. Generalisation (2)

Following on from the generalisation of the logic there needs to be corresponding generalisation of calculation of the total_score. I would use

total_score = 0;
for(int i = 1, i <= number_of_tests, i++) {
  total_score = total_score + test_score(i);
}

Now there are two for loops. Perhaps compiler optimisation can roll these up into one. I would do this explicitly and, at last, include a comment

// calculate lowest score and total of all scores
lowest_score = test_score(1);
total_score = 0;
for(int i = 1, i <= number_of_tests, i++) {
  if(test_score(i) < lowest_score)
    lowest_score = test_score(i);
  total_score = total_score + test_score(i);
}

and, just for comparison, we could save the first iteration of the loop with

// calculate lowest score and total of all scores
lowest_score = test_score(1);
total_score = test_score(1);
for(int i = 2, i <= number_of_tests, i++) {
  if(test_score(i) < lowest_score)
    lowest_score = test_score(i);
  total_score = total_score + test_score(i);
}

But, as I said before, I think this is unnecessary cleverness which obscures what is happening.

11. User interface niceties

The student has taken care on input to explain to whoever runs the program what the program does. But for the results, the adjusted grade value is all that appears. I think it would be an improvement to have a line which explains the results. Something like

cout << "The adjusted grade for your "
     << "five test scores is \n";

Finally there are a number of other points which make me feel uneasy. I would want to discuss these with the student's tutor / mentor / project leader as to whether the student needs additional help. These are:

To start with the question as asked, the code does not always find the lowest value because the value is only compared with its neighbours, not with the lowest value found so far. Obviously this can result in the value selected not always being the lowest.

Once this is fixed, I would expect the the program to work as expected, provided the input is exactly as expected, but it may not handle any unexpected variations in input gracefully and any extensions, e.g. to handle a different number of inputs, will require changes to the code.

After fixing the bug, I would start further improvements to the code by providing a more flexible input mechanism that allows the variable numbers of inputs. I would also break the program down into functions that will handle the individual tasks, so I might as well make this change by way of a new function, which I shall call input_data. I have defined input_data as:

std::vector<int> input_data(istream &in) {
  std::vector<int> data;
  while(!in.eof() && in.good()) {
    int val;
    in >> val;
    if(in.good()) {
      data.push_back(val);
    }
  }
  if(!in.eof()) {
    // We should only get here if there
    // has been an error on the stream
    cerr << "Input error reading data"
         << endl;
    exit(1);
  }
  return data;
}

[Watch out here. This function contains some pitfalls. For instance, what happens when EOF is right after the last number? Is it pushed into the vector? David]

The first thing you should notice is that this function returns a vector of ints. A vector is a structure provided the standard template library, which is capable of handling a variable number of elements. As vectors are defined by templates they may be used to contain any type you choose, in this case ints. Bear in mind that other list structures are available and a vector may not always be the best choice.

You may also notice that the input stream is left as a parameter, so that the function may read data from any input stream, e.g. a file, instead of being restricted to reading from cin.

Also needed is a way of indicating that the end of input has been reached. I have chosen to request an end of file character to indicate the end of the list. Again, this is not the only possible choice and a non technical audience may prefer something like entering the word "end", but this approach is simpler to code.

Another important point is that there was no error checking in your existing code. This function checks for errors and exits when there is an error on the stream. You should consider whether this code should continue when an error has occurred, in which case it will need some action to reset the stream to a good state before it will be able to read further.

I have updated the prompts to read

cout << "This program will gather test "
     << "scores and drop the lowest" << endl
     << "score, giving you the average of the "
     << "remaining scores" << endl << endl
     << "Please enter your test scores" << endl
     << "When all scores have been entered "
     << "please terminate the list" << endl
     << "with an end of file character "
     << "(^D in Unix, ^Z in Windows)" << endl;

It is important that when you modify a program, comments and text shown to the user are updated to match. If this is not done at the same time it will often be forgotten.

Many will argue that the use of endl for all line endings is inefficient. I prefer to always use endl for consistency, unless a program has a serious I/O performance problem.

The next task is to find the lowest value, which can be done by a simple function, but rather than writing our own, we can see that there is a suitable function already provided in the STL called min_element, which we can use:

std::vector<int>::iterator lowest
   = min_element(data.begin(), data.end());

Similarly we can use the STL function accumulate to produce an initial sum. To avoid confusion you really should not use the name "average" for the initial sum, that's somewhat confusing and your other names are similarly poorly chosen. I would suggest something like:

int sum = accumulate(data.begin(), data.end(), 0);
int adjusted_sum = sum - *lowest;
int result = adjusted_sum / (data.size()-1);

Other things I would change include changing using namespace std to using specific items from the std namespace and declaring variables where they are used instead of declaring them all at the start of the function. This leaves the final program looking like:

#include <vector>
#include <iostream>
#include <iomanip>
#include <algorithm>
#include <numeric>
using std::istream;
using std::vector;
using std::cin;
using std::cout;
using std::cerr;
using std::endl;
std::vector<int> input_data(istream &in) {
  std::vector<int> data;
  while(!in.eof()) {
    int val;
    in >> val;
    if(in.good()) {
      data.push_back(val);
    }
  }
  if(!in.eof()) {
    // We should only get here if there
    // has been an error on the stream
    cerr << "Input error reading data" << endl;
    exit(1);
  }
  return data;
}

int main() {
  cout << "This program will gather test "
       << "scores and drop the lowest" << endl
       << "score, giving you the average of the "
       << "remaining scores" << endl << endl
       << "Please enter your test scores" << endl
       << "When all scores have been entered "
       << "please terminate the list" << endl
       << "with an end of file character "
       << "(^D in Unix, ^Z in Windows)" << endl;
  std::vector<int> data = input_data(cin);
  std::vector<int>::iterator lowest
       = min_element(data.begin(), data.end());
  int sum = accumulate(data.begin(), data.end(), 0);
  int adjusted_sum = sum - *lowest;
  int result = adjusted_sum / (data.size()-1);
  cout << result << endl;
  return 0;
}

I'm sure lots of people can tell you why the output of your program depends on the order you enter the numbers, but I think it will be more useful to help you work it out for yourself. You can do this by looking at the values of the variables as you progress through the code. Here is a modified version of your code, I have added some more calls to cout, to show the value of lowest after each comparison.

#include <iostream>
#include <iomanip>
using namespace std;

int main() {
  int test1, test2, test3, test4, test5,
      average, averagescore, divide;
  cout << "This program will gather "
       << "five test scores and\n";
  cout << "drop the lowest score, "
       << "giving you the average\n";
  cout << "\n";
  cout << "Please enter Five Test scores\n";
  cin >> test1 >> test2 >> test3 >> test4 >> test5;
  cout << endl;
  int lowest = test1;
  cout << "lowest is " << lowest << endl;
          // test 1 is the lowest number unless
  if (test2 < test1) lowest = test2;
          // test 2 is smaller than test 1 unless
  cout << "lowest is " << lowest << endl;
          // test 3 is smaller than test 2 unless
  if (test3 < test2) lowest = test3;
  cout << "lowest is " << lowest << endl;
  if (test4 < test3) lowest = test4;
          // test 4 is smaller than test 3 unless
  cout << "lowest is " << lowest << endl;
  if (test5 < test4) lowest = test5;
          // test 5 is smaller than test 4.
  cout << "lowest is " << lowest << endl;
  average = (test1 + test2 + test3 + test4 + test5);
        // all test scores averaged together
  averagescore = average - lowest;
        // average score minus the lowest grade
  divide = averagescore /4;
        // average grade is then divided by 4
  cout << endl;
  cout << divide << endl;
        // final grade after division
  return 0;
}

If you run this version, with the values 80,84,60,100,90 it will print out

lowest is 80
lowest is 80
lowest is 60
lowest is 60
lowest is 90
81

Why has lowest increased to 90? What was the program doing just before the change? It was comparing test4 and test5. Since test5 is smaller than test4 the value of lowest is reset to 90. However you only want lowest to be reset if the new value (test5) is smaller than lowest. Here is a modified version of your code which should give the answer you want.

#include <iostream>
#include <iomanip>
using namespace std;
int main() {
  int test1, test2, test3, test4, test5,
      average, averagescore, divide;
  cout << "This program will gather "
       << "five test scores and\n";
  cout << "drop the lowest score, "
       << "giving you the average\n";
  cout << "\n";
  cout << "Please enter Five Test scores\n";
  cin >> test1 >> test2 >> test3 >> test4 >> test5;
  cout << endl;
  int lowest = test1;
        // test 1 is the lowest number unless
  if (test2 < lowest) lowest = test2;
        // test 2 is smaller than test 1 unless
  if (test3 < lowest) lowest = test3;
        // test 3 is smaller than test 2 unless
  if (test4 < lowest) lowest = test4;
        // test 4 is smaller than test 3 unless
  if (test5 < lowest) lowest = test5;
        // test 5 is smaller than test 4.
  average = (test1 + test2 + test3 + test4 + test5);
        // all test scores averaged together
  averagescore = average - lowest;
        // average score minus the lowest grade
  divide = averagescore /4;
        // average grade is then divided by 4
  cout << divide << endl;
        // final grade after division
  return 0;
}

Now I would like to mention a few other things I noticed while looking at your code.

Some of the variable names are misleading. The variable you call average is in fact the total, averagescore is a modified total - perhaps you could call it modTotal - and divide is the average.

I'm not sure why you have included iomanip - the code works without it. [Maybe he thought (mistakenly) it would be needed by std::endl. David]

If this was my code I would calculate the average as a float. For the 5 numbers you entered the average is in fact 88.5, so presenting 88 as your answer is fair enough, but if you had chosen say, 80, 84, 60, 91, 100, the average would be 88.75 and in many circumstances it would be better to round the answer up to 89. However I don't know the precise details of the problem you were asked to solve, so let's leave it as it is for now.

Finally I'd like to look at some ways of making your code more versatile. At the moment it requires exactly 5 inputs, it is relatively simple to make it work with any number of scores greater than one.

#include <iostream>
using namespace std;

int main() {
  int inValue, total, lowest, count, average;
  cout << "This program will gather two "
          "or more test scores and\n";
  cout << "drop the lowest score, giving "
          "you the average\n" << "\n";
  cout << "Please enter at least two test scores\n";
  cout << "End your input with a single full stop\n";
  cin >> inValue;
  lowest = inValue;
  total = inValue;
  count = 1;
  while(cin >> inValue) {
    ++count;
    total += inValue;
    if(inValue < lowest) lowest = inValue;
  }
  if (count < 2) {
    cout << "This program requires at least "
         << "two values" << endl;
  } else {
    average = (total - lowest)/(count-1);
    cout << average << endl;
  } 
  return 0;
}

Just one more improvement to go! In real life you may not have a user typing data in at the prompt - it may have come from a database, spreadsheet or a special user interface. So let's make your program into a function that returns the answer to whatever called it. We will assume that the calling program has already put the values into a vector.

#include <iostream>
#include <vector>
using namespace std;

int myAverage(vector<int> values) {
  int total, lowest, average;
  total = 0;
  lowest = values[0];
  for(vector<int>::iterator it = values.begin();
      it != values.end(); ++it) {
    total += *it;
    if(*it < lowest) lowest = *it;
  }
  average = (total - lowest)/(values.size()-1);
  return average;
}

The question as stated is slightly wrong. Here is the correction:

80 84 60 100 90 ==> 81 (incorrect!)
80 84 100 60 90 ==> 88 (correct)

[Good, you verified the student's statement, which was probably a typo. David]

Improvement #1: Fix the bug

The bug is in the if statements: instead of comparing adjacent test scores, each test score should be compared against lowest. The corrected code:

#include <iostream>
using namespace std;

int main() {
  int test1, test2, test3, test4, test5,
      average, averagescore, divide;
  cout << "This program will gather five "
       << "test scores and\n";
  cout << "drop the lowest score, giving "
          "you the average\n" << "\n";
  cout << "Please enter Five Test scores\n";
  cin >> test1 >> test2 >> test3 >> test4 >> test5;
  int lowest = test1;
  if (test2 < lowest) lowest = test2;
  if (test3 < lowest) lowest = test3;
  if (test4 < lowest) lowest = test4;
  if (test5 < lowest) lowest = test5;
  average = (test1 + test2 + test3 + test4 + test5);
       // all test scores averaged together
  averagescore = average - lowest;
       // average score minus the lowest grade
  divide = averagescore /4;
       // average grade is then divided by 4
  cout << divide<< endl;
       // final grade after division
  return 0;
}

Style: not bad, actually. Only a few minor nits.

Design: once the bug is fixed, his code gets the job done, albeit in a brute force sort of way.

Extensibility: here is the real shortcoming of this code. The number of test scores is fixed at 5. The number of scores we drop is fixed at 1. We can, of course, do better.

Improvement #2: Variable number of test scores

When I first read this problem, it screamed out to me that we should be using algorithms over a collection of test scores. Without changing the structure of the original solution too much, I came up with:

#include <algorithm> // for std::min_element
#include <deque>
#include <iostream>
#include <iterator>  // for std::distance
#include <numeric>   // for std::accumulate
typedef std::deque<int> Scores;
int AverageTestScore(Scores::iterator first,
                     Scores::iterator last) {
  int lowest = *std::min_element(first, last);
  int average = 0;
  average = std::accumulate(first, last, average);
  int averagescore = average - lowest;
  int divide = averagescore
                 / (std::distance(first, last) - 1);
  return divide;
}
int main() {
  std::cout << "This program will gather test scores "
       << "and\ndrop the lowest score, giving you "
       << "the average\n\nPlease enter Test scores, "
       << "followed by \"end\"" << std::endl;
  // store all the ints in cin into scores
  Scores scores;
  int test;
  while(std::cin >> test) {
    scores.push_back(test);
  }
  // Need at least two elements for this calculation
  if(1 < scores.size()) {
    int divide = AverageTestScore(scores.begin(),
                                  scores.end());
    std::cout << divide << std::endl;
  }
  else {
    std::cerr << "The number of scores needed is "
         << "at least 2; you only entered "
         << scores.size() << std::endl;
    return 1;
  }
  return 0;
}

Highlights:

Improvement #3: Variable number of low scores dropped

In order to do this, we need to sort the scores. There are a variety of different ways to do this. We could store them in a multiset. We could sort the entire collection. But this is overkill (in the sense of greater than linear time algorithms, such as Nlog(N)), as we don't need to sort the entire collection; we just need to group the low scores away from the high scores.

And there just happens to be an algorithm which does what we want: nth_element(...). What it does is put the nth element in the correct position as if the whole thing were sorted, and all the elements before the nth position are <= the nth element, and all the elements after the nth position are >= the nth element. Plus, nth_element(...) runs in linear time on average. However, nth_element(...) requires random access iterators, thus limiting the collection types to vector or deque, but not list.

#include <algorithm> // for std::nth_element
#include <stdlib.h>  // for exit
#include <deque>
#include <iostream>
#include <numeric>   // for std::accumulate
typedef std::deque<int> Scores;

int NonnegativeIntFromCin() {
  int value;
  if(!(std::cin >> value) || value < 0) {
    std::cerr << "Next time, please enter a "
         << "non-negative integer" << std::endl;
    exit(1);
  }
  return value;
}
int AverageTestScore(Scores::iterator first,
      Scores::iterator low, Scores::iterator last) {
  // Put the lowest test scores in [first, low) 
  std::nth_element(first, low, last);
  // Sum all the high [low, last) test scores
  int averagescore = 0;
  averagescore = std::accumulate(low, last,
                                 averagescore);
  int divide = averagescore / (last - low);
  return divide;
}
int main() {
  // Enter the number of low test scores to drop
  std::cout << "This program will gather test scores "
       << "and\ndrop the lowest score, giving you "
       << "the average\n\nPlease enter the number of "
       << "low Test scores to drop" << std::endl;
  int lowdropped = NonnegativeIntFromCin();
  // enter the test scores
  std::cout << "Please enter Test scores, followed "
       << "by \"end\""<< std::endl;
  Scores scores;
  int test;
  while(std::cin >> test) {
    scores.push_back(test);
  }
  // need at least 1 more score than number dropped
  if(lowdropped < scores.size()) {
    int divide = AverageTestScore(scores.begin(),
         scores.begin() + lowdropped, scores.end());
    std::cout << divide << std::endl;
  }
  else {
    std::cerr << "The number of scores needed is "
         << "at least " << lowdropped + 1
         << "; you only entered " << scores.size()
         << std::endl;
    return 1;
  }
  return 0;
}

The original functionality can be gotten by calling:

AverageTestScore(scores.begin(),
                 scores.begin() + 1, scores.end());

Improvement #4: Variable number of high scores dropped

That is another predictable extension, and it isn't hard to add. Basically, do the nth_element(...) trick on the high side of the collection as well, taking care not to resort the lowest scores.

#include <algorithm> // for std::nth_element
#include <stdlib.h>  // for exit
#include <deque>
#include <iostream>
#include <numeric>   // for std::accumulate
typedef std::deque<int> Scores;

int NonnegativeIntFromCin() {
  int value;
  if(!(std::cin >> value) || value < 0) {
    std::cerr << "Next time, please enter a "
         << "non-negative integer" << std::endl;
    exit(1);
  }
  return value;
}
int AverageTestScore(Scores::iterator first,
       Scores::iterator low, Scores::iterator high,
       Scores::iterator last) {
  // Put the lowest test scores in [first, low)
  std::nth_element(first, low, last);
  // Put the middle test scores in [low, high)
  std::nth_element(low, high, last);
  // Sum all the middle [low, high) test scores
  int averagescore = 0;
  averagescore
         = std::accumulate(low, high, averagescore);
  int divide = averagescore / (high - low);
  return divide;
}

int main() {
  // Enter the number of low test scores to drop
  std::cout << "This program will gather test "
       << "scores and\ndrop the lowest score, "
       << "giving you the average\n\nPlease enter "
       << "the number of low Test scores to drop"
       << std::endl;
  int lowdropped = NonnegativeIntFromCin();
  // Enter the number of high test scores to drop
  std::cout << "Please enter the number of high "
       << "Test scores to drop" << std::endl;
  int highdropped = NonnegativeIntFromCin();
  // enter the test scores
  std::cout << "Please enter Test scores, "
       << "followed by \"end\"" << std::endl;
  Scores scores;
  int test;
  while(std::cin >> test) {
    scores.push_back(test);
  }
  // need at least 1 more score than number dropped
  if(lowdropped + highdropped < scores.size()) {
    int divide = AverageTestScore(scores.begin(),
          scores.begin() + lowdropped,
          scores.end() - highdropped, scores.end()); 
    std::cout << divide << std::endl;
  }
  else {
    std::cerr << "The number of scores needed is "
        << "at least " << lowdropped + highdropped + 1
        << "; you only entered " << scores.size()
        << std::endl;
    return 1;
  }
  return 0;
}

The original functionality can be achieved by calling

AverageTestScore(scores.begin(), scores.begin() + 1,
                 scores.end(), scores.end());

As you can see, this isn't much different than my solution for improvement #3. Since it didn't involve much extra engineering or testing, I felt it was worth adding this functionality. Your mileage may very.

At this point I am done. There are other ways to extend this code (for instance, making AverageTestScore a templated function instead of hard coding its parameters); however, they tend to get in the way of readability and understandability for a student first getting started with the language (my target audience), and I'll leave those as an exercise for the reader.

"It's not fair!"

Inspector Slack was dozing peacefully in his favourite armchair after his Christmas dinner when he was interrupted by the familiar and unmistakeable sound of his children bickering.

"It's that computer game Sergeant Lake gave us for Christmas. Joy scored 80, 84, 60, 100 and 90 and got a grade of 88. I scored 80, 84, 100, 60 and 90 and only got 81", complained Matthew.

"Why is that unfair?"

"Because I got exactly the same scores, just in a different order".

"Just like those gloves I knitted for little Tommy Smith".

Slack ignored this remark from his house guest, a little old lady knitting quietly in the corner, and proceeded to vent his fury on his sergeant.

"Lake! I told him that Open Source Software would be no good. Bungling amateurs!".

"Did you say Open Source, Inspector?" inquired Miss Marple. "Doesn't that mean anyone can read the program? I should be most interested to see it, though I don't suppose I shall understand it."

Before Slack had time even to think "interfering old woman", Matthew had downloaded the source code from the internet and built it.

"See. If I enter my scores it gives me 81, but if I enter Joy's she gets 88."

"Oh dear!" exclaimed Miss Marple. "Do we have to type in the numbers every time we want to try it out?"

"I know," said Joy, "let's turn it into a function that can use any input stream. Then we can feed it test data in string streams and the real thing from standard input".

The children typed away busily, setting up a test function that used an assert to check the result of calculating a grade. With this rearrangement they could easily add other test cases too:

namespace {
  int CalculateGrade(istream &stream) { ... }
  struct TestCase {
    const char *scores;
    int grade; 
  };
  void CheckCalculateGrade(const TestCase &testCase) {
    istringstream stream(testCase.scores);
    assert(CalculateGrade(stream) == testCase.grade);
  }
  void TestCalculateGrade() {
    const TestCase testCases[]
      = { { "80 84 60 100 90", 88 },
          { "80 84 100 60 90", 88 } };
    const unsigned count
      = sizeof(testCases)/sizeof(testCases[0]);
    for_each(testCases, testCases+count,
             CheckCalculateGrade);
  }
}
int main() {
  TestCalculateGrade();
  cout << "This program will gather five test "
       << "scores and\n";
  cout << "drop the lowest score, giving you the "
       << "average\n" << "\n";
  cout << "Please enter Five Test scores\n";
  cout << CalculateGrade(cin) << endl;
  return 0;
}

When they tried it out, it duly reported an assert failure.

"How thoughtful," said Miss Marple with approval, "the program prints out what it is supposed to do. Do all programs do that?"

"Sadly not," sighed Matthew.

"It should really only be output when a command line option such as /?, -h or -help is set," added Inspector Slack with a punctilious air of authority.

"Dear me, my eyesight is poor these days, I seem to be seeing double looking at this program," fussed the old lady as she adjusted her spectacles.

"It's not your eyes," replied Joy, "it's just that every line has a comment repeating what the previous line does."

"Well, my dears, let's get rid of all that. There's absolutely no point in stating the obvious."

Slack bristled as he felt sure that Miss Marple had glanced knowingly at him when making this last point, but now she was again scrutinizing the code with an expression of sweetness and innocence on her face.

"Ah, that's much clearer. Now, surely what is named an average is really a sum, and what is called divide is actually the average."

Matthew reworked the code. "You always manage to work out which people aren't who they say they are. I bet that fixes it." He ran the program, but it still failed. Slack allowed himself a smile of satisfaction; this problem demanded professional detection skills.

"I thought these computers were supposed to make tasks easier, but I notice you still have to add up the test scores in one big sum," observed Miss Marple.

"We could use std::accumulate instead," answered Joy, "but we have to put the scores in a container first, like a vector." Miss Marple wasn't quite sure what a vector was. Her nephew Raymond West had once taken her for a very fast drive in his sports car which she was sure was called a Vector. With this fond memory she encouraged Joy to make the change. From this it became apparent that the number 5 would make a useful constant for the input loop, and could be used in the average calculation.

"Such a pity," muttered Miss Marple as she considered the simplicity of the accumulate statement.

"What's a pity?" asked Matthew.

"I was thinking, if only there were a nice function already available for finding the lowest value, similar to accumulate for finding the sum".

"But there is, it's called std::min_element." Matthew replaced all the if statements with min_element. The first attempt failed to compile, then he remembered it returned an iterator rather than a value. After de-referencing it the code built. Even better, the tests ran successfully too.

"I've got it!" cried Inspector Slack, who had been working feverishly with pencil and paper.

"It's okay, Dad, Miss Marple's already fixed it," Joy informed him.

Crestfallen, Slack looked at their code:

namespace {
  const int scoreCount = 5;
  int CalculateGrade(std::istream &stream) {
    vector<int> scores;
    for(unsigned n = 0U; n != scoreCount; ++n) {
      int score;
      stream >> score;
      scores.push_back(score);
    }
    const int sum = accumulate(scores.begin(),
                               scores.end(), 0);
    const int lowest
       = *min_element(scores.begin(), scores.end());
    const int average = (sum-lowest) / (scoreCount-1);
    return average;
  }
}

"Yes, but that doesn't explain why the original code didn't work. You see, the if statements compare each value to the previous value, when they should compare each value to the current lowest value."

"How clever of you, Inspector," said Miss Marple. Slack beamed with pride.

"However," she went on, "it seems to me that the really interesting question is why the mistake occurred. The programmer says in, now what did Joy call them? oh, yes, in the comments that he is using 'the equation' to drop the lowest number. He must have either been given the wrong equation or, more likely, noted it down incorrectly. I remembered I once made a mistake copying a knitting pattern from Mrs McGillicuddy, and made a pair of gloves for little Tommy Smith where the fingers came out in the wrong order."

Seeing the look of disappointment on the Inspector's face, and feeling guilty for outwitting him whilst enjoying his hospitality, she made a proposal. "I should very much like to see one of your magic tricks, Inspector, I do so enjoy them."

"I've been working on sawing the lady in half. Perhaps you'd like to lie down in that box over there while I fetch my saw," suggested Slack, with just the slightest hint of menace.

First the bug, the code above only works if the numbers after the lowest value are in increasing order, so for instance 80, 84, 100, 60, 90 works because the sequence 60, 90 is increasing, but 80, 84, 60, 100, 90 does not as 60, 100, 90 is not an increasing sequence (the problem description is the wrong way round it terms of the output of these sequences). The simplest correction would be not to compare each value in the series to the previous but to compare it to the lowest found so far.

int lowest = test1;
     // test 1 is the lowest number unless 
if(test2 < lowest) lowest = test2;
     // test 2 is smaller or
if(test3 < lowest) lowest = test3;
     // test 3 is smaller or
if(test4 < lowest) lowest = test4;
     // test 4 is smaller or
if(test5 < lowest) lowest = test5;
     // test 5 is smaller.

While this fixes the bug it does leave some aspects still wanting in the program.

To deal with some of the stylistic points first. The early declarations of average, averagescore and divide are unnecessary and should be shifted to where those variables are first defined. It would also be worth changing the name of average and averagescore, because the names do not match the meanings, perhaps total and amendedtotal respectively; divide could then be renamed averagescore which gives a better sense of its purpose. Another minor point is that the inclusion of iomanip is superfluous as nothing from this header is used. Personally I would also prefer to use a single std::cout reference for the printed block at the top, since this makes for fewer changes should we wish to send the output to a different stream in future (though this is a more marginal decision than the others).

A rather more major issue than those is the design of the program in that it does not easily allow extension. As more tests are added we would have to remember to update the code in seven places (the declaration of the test variables, the two references to five tests in the printed text, the cin statement, the comparison tests, the summation to produce the total score and the division to produce the average). The solution to this is to use one of the STL sequences to hold the scores.

While the initial thought might be to use std::vector or std::list, the arithmetic operations that we do on the sequence suggest a better choice in the form of std::valarray. This has convenient methods allowing us to find the minimum held value, the sum of the values and the length which are the three pieces of information we use. Implementing this change alters most of the program to produce something like:

#include <iostream>
#include <valarray>

int main() {
  const size_t number_of_scores = 5;
  std::valarray<int> scores(number_of_scores);
  std::cout << "This program will gather "
       << scores.size() << " test scores and\n"
       << "drop the lowest score giving you the "
       << "average\n\n"
            << "Please enter " << scores.size()
            << " test scores\n";
  for(size_t i = 0; i < scores.size(); ++i) {
    std::cin >> scores[i];
  }
  int averagescore = (scores.sum() - scores.min())
                         / (scores.size() - 1);
  std::cout << averagescore << std::endl;
  return 0;
}

A couple of notes. As you can see I've changed things round so that the number of scores is only set in one place and everything after that checks the the valarray size. I've also got rid of the intermediate variables for calculating the average as the method names on valarray express what the intent of the formula accurately.

The code is very close to working. There are two bugs: the first bug is in the code to calculate lowest. The code needs to compare successive grade results with the current v

Notes: 

More fields may be available via dynamicdata ..