Title: Student Code Critique Competition

While space is allocated for grades, the marks are actually read into test, which is an uninitialised pointer - leading to overwriting of storage. Pointers should be initialised before they are dereferenced.

The function allocatemem() is declared to return int*, but the definition returns int.

Nothing is done with the return code from allocatemem except to print a message. The code continues regardless of the success or failure of the allocation request. Actions need to be taken on return codes. As written, the code in allocatemem will not behave as the student expects if it is compiled with a conformant compiler. If new fails to allocate storage, it is required to throw a std::bad_alloc exception (3.7.3.1/3). To return a null pointer instead of an exception the user should call the nothrow variant of new. The <new> header must be included. At this point it is worth noting that the program does not free the allocated storage on exit. It is good practice to free-up dynamically acquired storage. Not all environments will clean-up after the program terminates. I'm thinking of some subpools in the MVS OS here.

In the function sortnums(), there is no good reason to declare temp in the for() and even if you did it should have been separated from int j=0 by a comma not a semicolon. The for() loop as written has four statements instead of three. The sortnums function doesn't sort the numbers either and there is a typo where i has been written as 1.

The argument is int scores which is never used and the loop goes over numberOfScores which is not defined anywhere - and even worse has two different spellings. The function needs an argument that defines the number of elements in the array. It is also defined to return an integer, but declared to return a pointer to an integer. The whole function is a mess and should be re-written.

The function main() should have been declared as int main() not void main(void). The C++ standard allows only int main() and int main(int argc, char *argv[]) as definitions (3.6.1/2).

There is no checking of user input. As a general rule input from a user should always be checked for validity. The coding of cin to retrieve values from the user is too trusting. If a user enters a non-integer value then the stream's fail bit will be set and the cin object becomes unusable. One solution is to call the clear member to reset the fail bit. This should be followed by the ignore member to discard any additional input from the stream.

Another minor flaw with the grade input code is that the student tampers with the loop control variable to handle incorrect input. While tampering with a loop control variable is not explicitly forbidden by the standard it is a sign of a poor program design. While not true in this case, there is a danger of such practices breaking the conditional test in the for() header.

The variable NoGrades is poorly named as it might be taken to mean that the student has no grades rather than holding the number of grades. Good naming standards are helpful to code maintainers.

The student has enclosed string literals in single quotes rather than double quotes in his/her dialogue with the user. Also as a matter of style, if cin and cout are being used it would be more consistent to use std::endl rather than '\n' as a newline^[1].

It is also not beneficial to use the pointer notation for the test array, when the array[subscript] format would have been clearer. There is an equivalence between *(test +i) and test[i], but the latter is easier to visualise - especially if you are a Fortran programmer :-)

The pointer returned from allocatemem is used to determine if the code proceeds with data processing or drops through to the final return. This removes the need to have multiple return points.

The getInput() function addresses the problems raised by the student's use of cin to retrieve input from the user. The getInput function requires that a valid range of input values be passed as arguments so that the user is required to enter a valid data type within the specified range. The getInput() function is a specialisation of the template code presented in response to SCC12.

I have snaffled a bubblesort() routine from http://leepoint.net/notes/cpp/algorithms/arrayfuncs/bubblesort2.html^[2]. The bubble sort routine is adequate for small amounts of input and is simple to understand and implement. The bubblesort routine sorts the array in place. The student did not specify what we were to do with the output, so I printed it just to demonstrate that the sort worked.

#include <iostream>
#include <new>
#include <climits>
using namespace std;
int *allocateMem(int);
void bubblesort(int *, int);
void getInput(const char *, int&, int, int);
int main(){
  int numGrades;
  int *grades;
  int i;
  getInput("Number of grades? ", numGrades, 1, 10);
  grades = allocateMem(numGrades);
  if(grades){
    for(i=0; i<numGrades; ++i){
      getInput("Enter Grade", grades[i], 0, 100);
    }
    bubblesort(grades, numGrades);
    for (i=0; i<numGrades; ++i){
      cout << grades[i] << endl;
    }
    delete [] grades;
  }
  return 0;
}


int *allocateMem(int amount){
  int *memory;
  memory = new(nothrow) int[amount];
  if(!memory){
    cout << "Error: Unable to allocate sufficient "
         << "memory." << endl;
    cout << "Program terminating." << endl;
  }
  return memory;
}

void bubblesort(int *x, int n){
// code suppressed use std::sort instead - Francis
}

void getInput(const char *prompt, int& r, int lower, int upper){
  bool error;
  do{
    cout << prompt << ": " << flush;
    error = !((cin >> r) && (r >= lower)
                         && (r <= upper));
    if(error){
      cout << "Please try again." << endl;
      cout << "Enter an integer between " << lower
           << " and " << upper << endl;
    }
    cin.clear();
    cin.ignore(INT_MAX,'\n');
  } while(error);
}

You have a few syntax problems - perhaps you were in a hurry? "More haste less speed" when it comes to syntax errors I'm afraid.

Firstly, C++ is fussy about the difference between a single and a double quote: ' and "; some other languages don't care. A single quote is for a single character, a double quote is for a string of characters. As it happens all your single quotes can be changed to double quotes.

Secondly, C++ is fussy about referring to variables in other functions. So sortNums tries to use the variable test from main but it can't see it - you need to pass this variable in as another parameter to the function.

Incidentally, sortNums is perhaps not the best name for the function since it does two jobs - it sorts the numbers but then prints them. I'd prefer writing two functions, sortNums and printNums, and so keeping code that changes the scores separate from code that prints them. You can then make the array constant in the printNums function that among other things prevents you accidentally writing code which changes it.

Thirdly, you aren't consistent and, alas, the compiler expects you to be completely consistent. It is also easier for you and everybody else if you use consistent names for things. For example, the number of grades in the program is called many names: NoGrades, grades, numberOfScores and numberOFScores in various parts of your code. You also have grades and test both referring to the same data.

Try to ensure that the names of variables are always the same - and this includes being in the same case. To make this consistency easier it is a good idea to adopt a simple policy on names - many people use a mixed case with first letter lower case like you have in numberOfScores. I've picked on one name and used it consistently in the program.

Another place where you must be consistent is that the types used for function returns and arguments must be the same in the function prototype and where the function is actually defined. The prototype for sortNums returns int* but the function itself claims to return int.

Lastly you have a small problem with the for-loop in sortNums - you can initialise multiple variables in the for-loop initialiser but if so you must separate them with commas. In your case I'd simple remove int temp=0; completely and put the int on the next line to declare and define temp in one go.

All being well you now have a body of code that compiles and does solve the problem you were set.

However we are not finished yet...compiling is not at all the same thing as working! It would be nice if your sample program worked and used standard C++.

Firstly, main is defined as void which many compiler still accept but it is an old usage. In standard C++ main always returns an int. You shouldn't need to put in a return-statement in main, the compiler ought to do this for you, but not all compilers do so put a return 0; just before the closing brace to be safe.

Secondly, sortNums is defined to return an int but doesn't return anything - and its return value is ignored where it is used! So I'd change sortNums to return void.

Thirdly, you have a very, very small loop. The for-loop in main has a spurious ';' which the compiler understands to mean 'do nothing' each time round the loop. Just remove it and then the loop will work as you expect.

And then lastly printNums doesn't print and sortNums doesn't sort! The for-loop in printNums uses 'test + 1' inside the loop where you wanted 'test + a'.

The sort is just broken but it is easily fixed. The solution here is to stop re-inventing the wheel. The problem didn't ask you to write a sort algorithm, so don't. Just use the standard library sort from the header <algorithm>.

The final code looks like this:-

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

// function prototypes
int *allocatemem(int);
void sortNums(int* grades, 
              int numberOfGrades);
void printNums(const int* grades, 
               int numberOfGrades);

int main(void) {
  int numberOfGrades;
  int *grades;
  cout << "Number of grades to enter: ";
  cin >> NoGrades;
  grades = allocatemem(numberOfGrades);
  if(grades == 0)return 1;
  for(int i=0; i<numberOfGrades; i++) {
    cout << "What is test grade # "
         << (i+1) << " ?";
    cin >> *(grades +i);
    if(*(grades +i) < 0 ) {
      cout << "Must be positive \n";
      cout << "Please enter Test #"
           << (i+1) << " correctly: \n";
      i-;
    }
  }
  sortNums(grades, numberOfGrades);
  printNums(grades, numberOfGrades);
  return 0;
}

int *allocatemem(int amount) {
  int *memory;
  memory = new(nothrow) int[amount];
  if(memory != 0 ) {
    cout << "We have memory \n";
    cout << "going back to main \n";
    return memory;
  }
  cout << "We do not have enough memory for "
          "this task \n";
  cout << "going back to main \n";
  return memory;
}

void sortNums(int* grades, int numberOfGrades) {
  std::sort(grades, grades+numberOfGrades);
}

void printNums(const int* grades, int numberOfGrades) {
  for(int a = 0; a<numberOfGrades; a++)
    cout <<*(grades + a)<<' ';
}