Title: Student Code Critique Competition

Layout

Apart from the obvious editing to fit the source in the columns, I would like to implore anyone that source code is a document. And documents should be readable, preferably to others. The code under scrutiny is sloppy in layout and lacks to convey what the intention is. More tongue-in-cheek, I think we can leave the time that there were language implementations that only allowed one-character variable names safely behind us. In my opinion, verbosity is not a bad thing, on the other hand, sometimes I myself go overboard.

A few style pointers are in order:

Idiom.

In 'Advanced C++ Styles And Idioms', Coplien tells us that if and when we do not intend to create types that are second grade citizens, we must adhere to the 'Orthodox Canonical Form'. This means that a default constructor, a copy constructor, an assignment operator and a destructor must be implemented. It is my tendency to explicitly specify all of them instead of relying on the compiler to generate the default ones for me, so I added the assignment operator. As for the destructor, I almost invariably make them virtual, just to prevent rude surprises if I derive another class from a class I created. Only in very special circumstances and duly commented in the source code I use a static destructor.

The main function usually is defined as:

int main(int argc, char* argv[])

but when not using those arguments, I would prefer to see:

int main(int, char*[])

but certainly not:

#pragma argsused
int main(int argc, char* argv[])

In the same vein, returning 0 at the end of main should be replaced with the more portable EXIT_SUCCESS.

[Please note that I would argue with almost all the above. Do not provide a virtual destructor unless you intend the class to be a base class - and think very carefully before deriving from a non-abstract base. I would prefer the signature int main() if command line arguments are not used because it is the long hallowed idiom to express that intent and return 0 is just as portable as return EXIT_SUCCESS. FG]

With respect to the ostream friend, I have a few remarks. Even in the eye of friendship, I tend to use accessor methods to obtain the inner value from the object to be streamed, but there is another little idiom I would like to present in this matter. If class A would be the base class for other classes, it is beneficial to separate the actual streaming from the body of the friend function and add that functionality to a protected virtual streamOn method [but why not make it virtual? FG] This makes the task of streaming much easier, because it is only necessary to overload the streamOn function in derived classes instead of rewriting the ostream friend boilerplate over and over again, so:

void Integer::streamOn(ostream& ostr) {
  ostr << getValue();
}
ostream& operator<<(ostream& ostr,
                    Integer const & obj) {
  obj.streamOn(ostr);
  return ostr;
}

Last but not least, if streaming an object to an ostream, adding text or an end-of-line to the streaming operation is generally a bad plan, because the user of the class then cannot decide how he will do output formatting. Every time he uses the streaming friend, he gets the text and the newline 'for free', possibly at a time that is inconvenient to him. Furthermore, it is very easy to forget to consume the text and the newline when at a certain time a corresponding input stream operator is written. Realizing that an ostream does not necessarily mean that the content of an object is printed on a console is important in this matter.

In the main function, a template is instantiated. I know that this is trivial code in some respect, but I prefer to use typedefs to create 'new' types based on templates, and while I am at it, typedef the iterators as well. So instead of:

vector<A> v;

I would prefer to see:

typedef vector<Integer> IntegerVector;
typedef IntegerVector::iterator
                       IntegerVectorIterator;
typedef IntegerVector::const_iterator
                       IntegerVectorConstIterator;

[If you are going to use those names I can see very little benefit from using typedefs. In my opinion a typedef should be used to create a more descriptive name for a type. Yours do not do that so skip them. FG]

and subsequently:

IntegerVector integerVector;

In a more general case, this even collapses to for example:

typedef vector<Rule> Rules;
typedef Rules::iterator RulesIterator;
typedef Rules::const_iterator
               RulesConstIterator;

[Now I agree about the first, but again I think the other two add nothing. FG]

in which case it is quite easy to change the container type to something different, for example, if rules are all of a sudden uniquely identified, into:

typedef set<Rule> Rules;

without having to change much in any code using the Rules type.

[and that is a good reason for the first typedef but not for the typedef applied to the iterators. FG]

Call them style idiosyncracies, I have developed some I guess. I have the tendency to use 'that' for the parameter name if I need to refer to another value of the same type, at least in the copy constructors and the assignment operator. Also, in streaming friends, I tend to name the ostream& parameter as ostr.

[Yes we all have our personal styles but we should always examine naming conventions and ask if they add to the readability of the code. FG]

Intent.

From what I gather from the source code, this is an exercise in reconnoitring the innards of a vector and the memory allocation strategies of the underlying operating system. I don't have a problem with stretching computers and operating systems. However, this type of code could be an indication that the student does not properly understand the true nature of object oriented programming at large or STL container classes in particular. Halfway through the main function the line between the concept of container classes and the technique employed by it is crossed. At a certain point in time, he seems to want to treat the vector container as an array of Integer objects, misusing the fact that vector::back() returns a const reference to the object in the container in order to create a pointer to that object.

At a certain point, the student wants to inspect the address of the object and uses a cast to output the address pointed to by the pointer to A. The cast is superfluous, and also wrong. If the intent was to convert the address to an integer number, on 32-bit architectures at least the cast would have to be towards an unsigned long, but since the cast is supefluous, simply streaming out the pointer would have yielded the address pointed to in hexadecimal notation. The simple:

cout << pa << endl;

would have sufficed. This leads me to believe that the student should again review how pointers are treated in C++, especially in the face of passing them to an ostream object.

The statement:

A* pa = &(v.back());

leads me to believe that the student must be taught the concept of ownership. It should be made clear to him that an STL container has ownership of the objects stored in it and that access to the elements of the container should be done through the methods the container provides. Awareness of ownership problems will make him a better software engineer.

Last but not least, the question why the program does not crash on line B and why the double deletion seems to be allowed. The case probably is that a vector allocates an array of objects to accommodate its elements. This means that vector<T>::pop_back() indeed calls the destructor of the element, but does not remove the storage space associated with it. In combination with the fact that we are dealing with a very simple class stored in the container, this could mean that even if the destructor is called, the memory is still there and is still used by the container. If the stored object was more complex, for example referring to dynamically allocated memory, the dereferencing probably would have gone awry as well as the double deletion of the object. For what it is worth, I tried this under gcc 3.2, and the second time the destructor is called through delete pa it is not even executed.

Finally.

I sort of tried to recreate the same problem here. The main code is not very much better in terms of properly handling the container, however, I adjusted a number of stylish and idiomatic flaws:

// Student Code Critique 22, Critiqued
#include <iostream>
#include <fstream>
#include <vector>
using namespace std;

class Integer {
public:
  Integer(int theValue = 0);
  Integer(const Integer& that);
  Integer& operator=(const Integer& that);
  virtual ~Integer();
  int getValue() const;
  friend ostream& operator<<(ostream& ostr,
                             const Integer& obj);
protected:
  virtual void streamOn(ostream& ostr) const;
private:
  int value;
};

Integer::Integer(int theValue)
    : value(theValue){
  cout << "ctor(" << value << ")" << endl;
}

Integer::Integer(const Integer& that)
    : value(that.value){
  cout << "cctr(" << value << ")" << endl;
}

Integer& Integer::operator=(const
    Integer& that){
  value = that.value;
  cout << "op=(" << value
       << ")" << endl;
  return *this;
}

Integer::~Integer() {
  cout << "dtor(" << this
       << ")" << endl;
}

int Integer::getValue() const {
  return value;
}

void Integer::streamOn(ostream& ostr) const {
  ostr << getValue();
}

ostream& operator<<(ostream& ostr,
    const Integer& obj) {
  obj.printOn(ostr);
  return ostr;
}

typedef vector<Integer> IntegerVector;
typedef IntegerVector::iterator
IntegerVectorIterator;
typedef IntegerVector::const_iterator
IntegerVectorConstIterator;

int main() {
  IntegerVector integerVector;
  for(int i = 0; i < 3; i++)
    integerVector.push_back(Integer(i));
  Integer* integerpointer
    = &(integerVector.back());
  cout << "integer value = "
       << *integerpointer << endl;
  cout << "integer address = "
       << integerpointer << endl;
  cout << "call integerVector.pop_back()"
       << endl;
  integerVector.pop_back();
  cout << "integer value = "
       << *integerpointer << endl;
  cout << "integer address = "
       << integerpointer << endl;
  delete integerpointer;
  return EXIT_SUCCESS;
}

[I made some minor corrections to Ruurd's English and added rather more comments than I normally do somewhat in the style I would use were I conducting a seminar. I am aware that naming style is both individual and varies from one national group to another. However names should add value and, in my opinion, overly long names work to destroy the shape of code. It is no accident that mathematicians prefer very short names. The skill is in finding the point of balance.

My final point is that while I agree with the provision of streaming functionality through a member function, I would make it public and abandon the friend declaration of the streaming operator <<. There is still a tendency to overuse friend declarations. FG]