Title: A Code Critique

Not infrequently appeals from students for help appear in the better quality newsgroups. We have a general policy that we will not do a student's homework. However, where a student is making a genuine attempt and either post's their code or asks a specific question the resident gurus are more lenient.

In mid-January I noted such an appeal and as it was actually posted to an inappropriate NG I contacted the student directly. What I then learned disgusted me. This was not a first time student struggling with a C++ 101 course but a student who already had rather more relevant experience than the course required. The course was a pre-requisite for starting an MSIS course.

In the sidebars I provide two lightly edited emails from the student. Do you feel as angry as I do that any academic establishment should tolerate the work of such a clearly inadequate employee?

By the way does anyone know how to provide some cursor control in a console application? Using combinations of various control characters (\b, \t, \v and \n) allows forward navigation and moving backwards along a line but is there any way to move up a line? If you know, please tell me. Even better if you know of a library that provides cursor control in a console app window please pass it on. I asked the student concerned to let me see her source code so that I could pass it on to the Harpist for a code review. She kindly did so though I have removed anything that would identify the author or university.

Over to 'The Harpist.'

I am going to do a complete critique on the code provided, but do so in stages. As always, I would welcome comments from readers (just send them to Francis). I am going to tackle this in an unexpected order. First I am going to look at the application code.

#include <iostream>               //for cin and cout
#include "student.h"             //for class student

using namespace std;

int main(){
  int counter;                      //counts number of entries
  counter = 0;                    //initialize counter to zero
  
  char go;                          //to control looping
  char crap[30];              //to clear the buffer???

  do  {                                 //Start loop so can repeat entry of 5 students 

  Student s1;                        //instantiate object
    s1.GetSID();                  //call member functions
    s1.GetName();
    s1.GetPhone();
    s1.GetAddress();
  Student s2;                           //instantiate object
    s2.GetSID();                 //call member functions
    s2.GetName();
    s2.GetPhone();
    s2.GetAddress();

    s1.Show();                       //show the student info for each object
    s2.Show();
  <snipped>
  cout << "Do you wish to continue? (Y) or (N)o ";  //prompt for end of data entry
  cin.get (go);                                         //read input, compare to see if stop
  cin.getline (crap, 35);                       //I dunno, but it makes it work!!!

  }
  while ((go == 'y') ||(go == 'Y'));                  //test for quitting value
  return 0;  
}

#include <iostream>             
#include "student.h"
       
using namespace std;
void initialiseStudent(Student * s_ptr){
    s_ptr->GetSID();               
    s_ptr->GetName();
    s_ptr->GetPhone();
    s_ptr->GetAddress();
}

int main(){
  char go;                    //to control looping
  do  {                          //Start loop so can repeat entry of 5 students 
    Student s1;                    //instantiate student
    initialiseStudent(&s1)   // and initialise
    Student s2;                    // likewise for 4 more
    initialiseStudent(&s2) 
    Student s3;                    
    initialiseStudent(&s3)   
    Student s4;                    
    initialiseStudent(&s4)
    Student s5;                    
    initialiseStudent(&s5)
    s1.Show();                       //show the student info for each one
    s2.Show();
    s3.Show();
    s4.Show();
    s5.Show();
    cout << "Do you wish to continue? (Y) or (N)o ";  //prompt for end of data entry
    cin >> go;                                         //read input, compare to see if stop
    while(cin.get()!=  '\n');         // flush input buffer
  }   while ((go == 'y') ||(go == 'Y'));              //test for quitting value
  return 0;
}

inline void flushInput(istream & input) { while(input.get() != ''\n';}

int main(){
  do  {                                //Start loop to get data in blocks of 5 students
    Student s[5];                 //instantiate array of five students
for(int i = 0; i<5; ++i) initialiseStudent(s+i);
for(int i = 0; i<5; ++i) s[i].show();
    cout << "Do you have another five to do? (Y)es or (N)o ";
    char getMore;                    // moved to point of first use
    cin >> getMore;                                   
    flushInput(cin);            
  }   while ((getMore== 'y') ||(getMore== 'Y'));            
  return 0;  
}

Now let me have a look at the header file. In working on this I will want to limit the changes to the application code. Here is the original:

class Student
// a class for organizing student information
{
public:
  Student();                        //constructor
  void GetSID();                //allows entry of student ID  
  void GetName();             //allows user to enter student name  
  void GetPhone();            //allows user to enter phone number  
  void GetAddress();         //allows user to enter address  
  void Show();                    //displays student info
private:
  char number[7];              //for student ID
  char phone[13];              //for student phone number
  char name[30];               //for student name
  char address1[30];         //for student's street address
  char address2[35];         //for city, state and zip
};

One problem with this code is that it is non-idiomatic. C programmers use get-functions in this way, C++ programmers do not. I am going to skip over whether this class should be value based (have publicly available copy constructor and copy assignment). In addition my design is going to be simplistic. In other words I am not trying to write production code nor the kind of code that and experienced programmer might.

#ifndef STUDENT_H
#define STUDENT_H      // standard sentinel to prevent multiple inclusion 
// class for acquiring student information
class Student {
public:
  Student();
  Student & initialiseStudent();
  Student const & displayStudentData(ostream & output) const;
  void Show() const { displayStudentData(cin); }
protected:
  Student & id()
  Student & phone()
  Student & name()
  Student & address()
private:
  enum MaxLengths { id_len = 7, phone_len = 13, name_len =30, addr1_len =30, addr2_len =35};
  char id_m[id_len];
  char phone_m[phone_len];
  char name_m[name_len];
  char address1_m[addr1_len];
  char address2_m[addr2_len];
};
#endif     // end of conditional inclusion

The first thing you will notice is that I have added an industry standard sentinel to ensure that this file is not multiply included into some other file. Multiple inclusion would cause redefinition errors.

I guess the instructor has never said anything about const member functions. However he should have done because it is important to develop the correct habits right from the start. Even if the explanation is very brief. Any member function that does not conceptually modify the state of an object should be qualified as const (by placing const after the closing parenthesis for the parameter list.

This is not the place to explain the return type for member functions, but there are good reasons for replacing void returns with references or const references to the class type.

I do not want applications to rely on being able to change individual pieces of data so I have made those functions protected (that will make more sense when the course moves on to inheritance)

I have provided a new function to write the data for a student from an istream, and then defined show() to call that function with cin. The advantage of the new function is that you now have something that can be used when you want to write the data to a file, send it to a printer etc.

Now why do you think I have provided that enum? It follows the rule that information should only be provided once. If, at some later stage, you decide you need to provide more space for the telephone number you only need to change the value provided in that enum for phone_len. Again, this kind of idiom can be introduced early with a full explanation to follow later. You do not need to know about enumerated types to use this idiom correctly. It is private because I do not want other code to rely on these values.

Now I can go back to the application and remove initialiseStudent() and change the line that uses it in main() to: for(int i = 0; i<5; ++i) s[i].initialiseStudent();

When the course has covered default parameters initialiseStudent() can be enhanced to work from other streams while still retaining the same default behaviour we are supplying here.

My definition of class Student is a bit longer than the original but it has more potential for development and is closer to the way good programmers write.

Here is the original:

#include <iostream>
#include <string>
#include "student.h"
using namespace std;
//definitions of member functions for class Student
Student::Student()         //Constructor
{   }
void Student::GetSID()    //Get student number
{
  char num[7];
  cout << "Enter six digit student number ";     //prompt for student number
  cin.getline (num, 7);                            //read the input
  strcpy (number, num);                            //copy input to data member
}  

<3 Similar functions snipped>

void Student::Show()    //display the student info 
{
  cout <<"\n\n";
  cout << "Student number        " << number <<"\n";   //show the student number
  cout << "Name:                 " << name <<"\n";                   //show the name  
  cout << "Phone number          " << phone <<"\n";      //show the phone number  
  cout << "Street address        " << address1 <<"\n";   //show street address  
  cout << "City, state, and zip  " << address2 << "\n";  //show city, state. etc
  cout << endl;
}

Again we have a chance to factor out common code. Look at the following function. I have declared it as static so that it is limited to the current file. I am well aware that using such things as unnamed namespaces are preferred but if students are being taught by instructors that are half as bad as the one in question here, we need to keep to as little new material as we can while doing a reasonable job.

static void init_array_of_char (char const * prompt,  char * destination,  size_t length) {
  cout << prompt;
  cin.getline(destination, length);
  while (cin.get() != '\n');   // flush input buffer
}

Now my four protected functions can be written as:

Student & Student::id(){
  init_array_of_char("What is the student number?", id_m, id_len);
  return *this;
}
Student & Student::phone(){
  init_array_of_char("What is the student's telephone number?", phone_m, phone_len);
  return *this;
}
Student & Student::name(){
  init_array_of_char("What is the student's name", name_m, name_len);
  return *this;
}
Student & Student::address(){
  init_array_of_char("What is the student's street address", address1_m, addr1_len);
  init_array_of_char("What is the student's City, State and zip-code", address2_m, addr2_len);
  return *this;
}

Now let me show how easy it is to use these to provide initialiseStudent()

Student & Student::initialiseStudent(){
  return id(), phone(), name(),  address();
}

That is a comma separated list so each function is evaluated in sequence and the return value of the last one is returned as the value of the function.

The next thing to consider is that constructor. The problem is that it leaves the object in an uninitialised state. If the show() function is ever called on an uninitialised Student object you may get bizarre output as the arrays will hold garbage (and may not even be zero terminated. It would be much better to use the following definition:

Student::Student():id_m(""), phone_m(""), name_m(""), address1_m(""), address2_m("") {}

to ensure that all the arrays have been set as empty strings.

And finally let me deal with my displayStudentData():

Student const & Student::displayStudentData(istream & input)  {
  input    <<"\n\n"
         << "Student number :      " << number <<"\n"
        << "Name:                 " << name <<"\n"           
        << "Phone number          " << phone <<"\n"  
         << "Street address        " << address1 <<"\n" 
        << "City, state, and zip  " << address2 << "\n"
        << endl;
}

Note that the way I have developed my Student class means that all the data, its capture and display have been encapsulated. The application neither knows, nor has any need to know, exactly what data is used and how it is structured. If I were teaching this course I would want to strongly emphasise this (perhaps by asking the student to add a datum, or separate city from state and zip code.

I think that just about wraps it up.

I am going to ask Francis to send this column to the original code writer and invite her questions and comments. If she sends those back to Francis in time I will try to add a response to this column.

Francis forwarded the student's first response to the above. Her comments are in italic Times Roman while mine are in Arial. I first added the following injunction:

OK. I hope this helps a little. Though as Steve Heller says in 'Whose Afraid of C++' programming has to be studied, you cannot expect to learn by simply reading once.

Now on with my comments on your response.