Title: Student Code Critique Competition 10

Sadly there was only a single entry this time. Even sadder was that it came from someone who teaches C++ courses at a community college. I am exercising my editorial right to protect contributors by suppressing his name and other details that might identify him. The reason is that his code exhibits so many flaws and his critique (in my opinion) is so far off beam that while I will happily award him the prize (he deserves it because he made the effort to enter, which puts him ahead of the rest) I cannot in all honesty hold up his entry as a good example. I only hope he understands. I have commented his critique. If readers think I should not have done, I agree that it was a close call. I think if I had had other entries I would have quietly returned this one. Anyway, here is the original code slightly cleaned up):

// Design Student Class,
#include <iostream.h>
class Student //specify an object
  {
  private:
    int subjcode[10];//array of subject codes
    int mark[10]; // array of marks
    int num_marks;//number of subjects taken
  public:
    Student(){}
//default constructor,no args,does nothing
    Student(int grd, int cd) 
//constructor(two args)
    {  subjcode= cd;
      mark = grd;
    }
    void showstudent()//display data
    {
      cout<<"\nYour Subject Code is:"
                     <<subjcode";
      cout<<"\nYour mark is:"<<mark;
    }
};

void main()
{
  int count=2;//count controls for loop
  class Student[count];//array for 2 students
    for (int n=0; n<2; n++)
    {
      student[n].showstudent();
//prints array
    }
  Student[0]();
//no marks so use default constructor
//First element of array is subject code,
//2nd element is grade
  Student[1](0,87);
  Student[0](0,78);
  Student[0](1,83);
  Student[1](2,66);
  Student[1](3,92);
}

The Student Code Critique Competition 9 column in C Vu Issue Volume 13 - No. 9 starts off, "Something short and confused (confusing?) this time, and in C++". "Confused" is certainly appropriate for the author of this code. It is obviously from a rank amateur at C++. But it looks familiar to me since I teach ... The errors spotted in this code are pretty common mistakes for beginners.

The problem given to us by Francis Glassborow is to write what you think was the spec to which the student was responding. Here's what I think it was:

Short - but I think adequate.

FG: Maybe, but I wonder if the task was:

Now. et us see what the student did wrong in designing this class. First of all, the braces do not line up. The student put the closing brace for each block of code in line with the opening brace most of the time, but the opening and closing braces are not lined up where I think they should be. For instance, the very first opening brace for the class should line up under the 'c' of class as below: I know, some programmers put the opening brace on the same line as the class header, but I prefer the opening brace on the next line under the first letter of the header.

FG: Sorry, but this a typical remark from some teachers. The layout of the program can help with avoiding some types of errors but is a red-herring here. Indeed the student has obviously tried to use a sensible and consistent layout and criticism of small failings are unhelpful and distract from the real problems.

Then the keyword private: should be indented two or three spaces on the next line. Thus, we have:

class Student
{
  private: 

The next really big error, in my opinion, is making the subjcode data member an array. I think it should be

FG: And I think the writer should have assumed this was not an error and have asked why there should be an array of subject codes. Instead he should have been thinking about ways to couple codes and marks (modern C++ instruction might suggest the use of the pair<int, int>, or even pair<Subject, Mark> because even if subject codes look like numbers they are not integers (you cannot do arithmetic with them). On the same theme, some container such as a vector would have advantages. (You could have add_subject(int, int) that would simply use push_back() to allow the vector to grow naturally, another possibility would be a map - the instructional potential is considerable)

int subjcode; // no array.

Ditto for

int mark;   // it should not be an array.

FG: I think the above are good examples of how not to use comments even in instructional code. They mean nothing without the context of the supposedly erroneous code.

Just instantiate an object as an array in main().

FG: Given the assumption as to the problem, I do not see how this is relevant.

The next error is defining the data member num_marks. It is declared but not used anywhere in the program. Why have it if it is not used? Actually, it's not even needed. If you want to know how many real grades you have in the array that you eventually instantiate, just assign the number zero to subjcode, and walk through the array (instantiated in main()) while subjcode != 0. Then you can print out the structures as long as the subjcode is not equal to zero.

FG: And all the while there is the 'better' option of using an STL container that will know its own size. But even if you were using an array, caching the size instead of repeatedly walking it makes perfectly good sense and is not an error, or even, in my opinion, a design error. The only mistake here is that the student has not tested the code. But given the very real problems that are to come, that is not surprising.

The default constructor has comments of "Default constructor, no args, does nothing"

In my opinion, if it does nothing, its useless, don't bother typing it in! Just because the default constructor takes no arguments doesn't mean it should do nothing. In fact, it should at least initialize the data members to some default value (zero). The default constructor is called to instantiate an array of 10 objects of type Student. My default constructor looks like this:

Student()
{
  subjcode = 0;
  mark     = 0;
}

FG: Now this is particularly interesting. The student could not do anything (well apart from zero num_marks) in the constructor because you can only default initialise an array. The student's constructor should have read:

Student(): num_marks(0) {}

However the writer of this critique could, and should have done better:

Student():subjcode(0), mark(0) {}

Another big mistake in this program is that there is no assignment function other than the constructor! The class needs a way of changing the data after the objects are created, so I added the following method to the class:

void assigndata (int cd, int grd)
{
  subjcode = cd;
  mark     = grd;
}

FG: It maybe poor design, but a perfectly good assignment already exists once a constructor with two parameters is provided:

void example(){
  Student s;
  S = Student(1234, 57);
}

so it is hardly fair to classify this as a big mistake. And is still just burdening the student with minor errors that do nothing to tackle the root of the problem. In fact the student is likely to have lost interest before the real problems are addressed.

There is also an error in the constructor that takes two arguments, Student::Student(int grd, int cd); This constructor is ok except the data members are reversed in order from the class definition. Be consistent and keep the same order. It's a lot easier on anyone reading the code, even the author. Write:

Student(int cd, int grd)

The constructor works well otherwise.

FG: Again, this is not an error. And it emphasises the wrong thing. Consistency has nothing to do with it (the internal order is private and irrelevant to the public interface.) The reason for putting the subject code first then the mark is exactly because that is the way we would normally think about it subject:mark not mark:subject

The student who wrote this code has made the classic beginner's mistake of not declaring an object of the type class Student. The reason his code does not work is because he has no objects to work on. What should go in main() is something like the following:

Student JoeStudent[10];

The above code gives you an array of ten objects of type Student. It is an easy exercise to fill the array using a while loop or fill the array by "hard coding". Here's my final program:

FG: At last we come to the fundamental problem, all the rest is fog until this issue has been tackled. But not that the writer still has an 'error' in his code because he is using the pre-standard headers. A small issue, unless you are teaching and choose to pick on so many things that are purely matters of style. Note that I have changed the layout for compactness.

#include <iostream.h>
class Student {// define the class
  private:
    int subjcode;  //subject codes array
    int mark;  
// marks for each subject code array
  public:
    Student(){
      subjcode = 0; mark    = 0;
    } 
// default constructor, no args, does nothing
    Student (int cd, int grd){
// constructor  (two args)
      subjcode= cd;   
      mark = grd;
    }
void Student::assigndata(int code, int grade){
      Student::subjcode = code;
      Student::mark =  grade;
  }
    void showstudent() { //display data
      cout<<"\nYour Subject Code is:" <<subjcode;
      cout<<"\nYour mark is:"<<mark<<endl;
     }
};   // end of class Student

int main() // should be int to conform to ANSI standard
{
    Student JoeStudent[10];  
//declare an array of ten Student objects
// assign data to part of the array for
// testing
    JoeStudent[0].assigndata(1234,100);
//assign data to one object in the array
    JoeStudent[1].assigndata(7562, 90); 
    // print out the data
    for (int n=0;n<10;++n)    {
        if(JoeStudent{n].subjcode != 0)
          JoeStudent[n].showstudent();
//prints array subscript if subjcode > 0
    }
    return 0;
}  // end of main

And here is how that code could have been written. Note that inline definitions are far more questionable than any matters of code layout

In Student.h

#ifndef STUDENT_H
#define STUDENT_H
#include <iostream>
class Student {
  public:
    Student(int subject_code=0, int mark=0)
    void showstudentdata(std::ostream& =std::cout) 
// copy ctor, copy assignment and dtor work 
// correctly
  private:
    int subjcode, mark;  
};
#endif

In test.cpp

int main() {
  Student studentrecord[10];  
  studentrecord[0] = Student(1234, 100);
  studentrecord[1] = Student(7562, 90);
  for (int n=0;n<10;++n){
    studentrecord[n].showstudentdata();
    std::cout << '\n'
  }
  return 0;
}

In student.cpp

#include "student.h"
Student::Student(int s, int m): subjcode(s),mark(m) {}
void Student::showstudentdata(std::ostream & out){
  if(subjcode)
    out<< "Subject number " << subjcode 
       << " Mark: " << mark;
}

Now please study the above two programs. Notice that the contestant's code is cluttered with comments that add little if anything. Mine is comment free (does it need comments?, well there is one place I might have added a comment for a student) Note also that the contestants code tries to access private data because the test for 'real data' is at the wrong place. He rushed his solution to meet a deadline, but had he instinctively tested where it mattered, he would not have made the mistake.

OK let us try something slightly different this time. Go back to my interpretation of the student's task (a class to manage the subjects taken and marks attained by a single student) and write a model answer. All types you need should be properly implemented (note that subjects and marks are types) in an appropriate fashion. Now explain clearly your reasons for the different design decisions you made.

Entires by July 15 please.