Title: Student Code Critique Competition

In an effort to understand the student code presented, I started with the name of the function. comp_stu. My guess is that this refers to "compare students." A quick look at the declarations suggests that 3 students are to be compared (all the grades are arrays that can hold 3 entries). This points out an immediate problem with the name array, which will only hold one name of up to 20 characters. The variable should be correctly declared as:

char name[3][20] /* 3 names, up to 20
             characters each */

The file "grades.data" is opened without confirming that the fopen command was successful. A check to see if the file is opened correctly should be incorporated as:

if ((grades_data = fopen("grades.data", "r"))
               == (FILE *)NULL) {
 fprintf(stderr, 
    "comp_stu: Unable to open grades_data\n");
 return; }

In this way, the programmer checks to be certain that the file has been opened successfully.

The function contains two calls to fscanf, that are both the same. By properly structuring the loop, the two calls can be combined into one. The loop as it exists, tests val, which is the return from fscanf. The return value of fscanf is the number of items scanned in from the input stream (EOF only if the input ends before the first conflict or conversion). Since the loop really is checking for the end of the stream, it would be better to use the feof function. As noted above, however, no more than 3 students can be compared, so this needs to be incorporated into the loop as well. It should be obvious that if more than 3 students are in the input file, it doesn't matter, as no more than three will be evaluated. Thus, the new loop condition should be:

while ((!feof(grades_data)) && (i < 3))

This checks for the end of file and checks the student count to be sure no more than three students are processed. In general I prefer a better variable naming convention than i but it is not terribly unreasonable as this variable is actually a counter.

Looking at the existing while loop, it is clear that the student wishes to compare students and print them out in order. It is necessary, however, to read in all the students, then perform the comparison. Thus, the comparison needs to be removed from the loop.

Since the loop condition above addresses the file, it is clear that the loop should read in the students. This suggests that the next statement should scan in the student information. The scan statement used in the existing program is basically fine for this purpose if it is fair to assume that the data is presented exactly in the form expected by the scan statement. In the last C Vu, Mr Stroustrup went to great lengths to examine the difficulty of reading in strings and explaining how difficult the C code for doing so is. For the sake of simplicity (and the premise of this being a true student project), I will assume that the teacher has guaranteed that the data is in the correct format or it should be considered a catastrophic error.

The first fscanf shown in the program needs a couple of simple modifications to perform more appropriately. The number of characters in the name needs to be constrained so that the array is not overrun. Thus the new call would be:

val = fscanf(grades_data, "%19s%d%d%d%d%d",
   &(name[i][0]), &(test_1[i]), &(test_2[i]),
   &(test_3[i]), &(assign[i]), &(final[i]));

It is important, however, to check the return value from fscanf to make sure that all the variables were read in! This requires the following additional code after the fscanf function:

if (val != 6) {
 fprintf(stderr, 
  "comp_stu: Error reading from grades data\n");
 (void)fclose(grades_data);
 return; }

The loop is almost complete, but it is necessary to increment the counting variable, i, and that will end the loop.

i++;

With the students read in, it is no longer necessary to keep the input file open. I like to close files right away so that there are no unused file descriptors hanging open inappropriately. fclose returns a value (0 on success) and there is rarely anything to be done if a file won't close, but just in case there is a problem, it is worthwhile to check the return value and handle it properly:

if (fclose(grades_data) == EOF) {
 fprintf(stderr, 
"comp_stu: Warning unable to close grades file\n");
 fprintf(stderr, 
  "comp_stu: Error number %d\n", errno); }

Now it is time to compare the students. Possibly, however, fewer than 3 students were read in, so it is important to check the number of students read in and use that to guide the comparison.

A different sorting function is performed depending on the number of students read in. This calls for a switch statement which works based on the number of students:

switch(i)

The first case is simple, if only 1 student was read in, then only 1 student needs to be printed. While the printf command in the student program has nothing inherently wrong with it, printing single spaces n times using the %s command is overkill. The spaces can be inserted directly. In addition the array index must be used consistently and putting sizing information for the width of the fields will keep columns consistent (The -19 left justifies the string in a 19 character field). Finally, the integer values should not be made into pointers:

case 1: { printf("%-19s   %6d  %6d  %6d  %6d  
%6d\n", &(name[0][0]), test_1[0], test_2[0], 
test_3[0], assign[0], final[0]);
     break; }

If two students were read in, then the larger name should appear first. This can be determined by looking at the "a < 0" condition in the original program. If the first string is smaller than the second string (strcmp returns a negative), then the original programmer prints the second string first (or at least starts to print the second string first). The two student case is also easily handled:

case 2: { 
if (strcmp(&(name[0][0]), &(name[1][0])) < 0) {
  printf("%-19s   %6d  %6d  %6d  %6d  %6d\n",
    &(name[1][0]), test_1[1], test_2[1], 
    test_3[1],assign[1], final[1]);
 printf("%-19s   %6d  %6d  %6d  %6d  %6d\n",
    &(name[0][0]), test_1[0], test_2[0],
    test_3[0],assign[0], final[0]);
} else {
/* code in reverse of the above order snipped */
break; }

At this point, it is almost frustrating to continue to try to use the style of code created by the student. On the other hand, it seems that the student will learn more from the review process by seeing the code back in his/her own style. Given that assumption, the case of three students must be handled. Below is a table that shows the possible permutations on three students:

Given the sequences in the table, it will be possible to find the proper order of the students by doing up to three strcmp calls. These permutations hold true even if a and c, for example are the same (True, False, False and the associated rearrangement would work). At this point, it should be obvious that there will be six sets of printf statements. There is no question that this is inefficient code, but I think that using a sort call or something like that would simply obfuscate the errors made by the student programmer rather than elucidate them. Thus, the case statement for three students is (the "printf" calls are omitted for clarity):

case 3: { 
if (strcmp(& (name[0][0]),& (name[2][0])) > 0){
  if (strcmp(&(name[1][0]), &(name[2][0])) > 0) {
    if (strcmp(&(name[0][0]), &(name[1][0])) > 0) {
        /* print 0 then 1 then 2 */
    } 
    else {
        /* print 1 then 0 then 2 */
    }
  } 
  else {
        /* print 0 then 2 then 1 */
  }
} 
else {
  if (strcmp(&(name[1][0]), &(name[2][0])) > 0) {
       /* print 1 then 2 then 0 */
  } 
  else {
    if (strcmp(&(name[0][0]), &(name[1][0])) > 0) {
        /* print 2 then 0 then 1 */
 } 
  else {
        /* print 2 then 1 then 0 */
  }
 }
}
break; 
}

There remains the possibility that no students were read in from the grades file, so that should be handled by the default in the switch statement:

default: { 
  fprintf(stderr,
        "comp_stu: No students found\n");
  break; 
}

With this the case statement is complete and the function is ended with a return statement.

Sometime shortly Brett will be receiving a copy of Standard C++ IOStreams and Locales by Angelika Langer and Klaus Kreft (reviewed in the current issue of Overload).