Title: Student Code Critique Competition 36

Firstly, I should apologise for the poor quality of this critique. I've only recently started using the STL, so my understanding is limited. I'm looking forward to seeing and learning from more expert critiques.

My immediate reaction upon seeing this code was confusion: the comment says that Reports is a vector, but the code indicates that it is a map. While both are containers, map is associative (in which you reference elements by a key), while vector elements are referenced by position. Given the presence of GetReport(int nReport), I shall assume that the comment is right, and that the code is an attempt to induce map to behave like vector.

It is possible that the goal is to have a type that is a map in some contexts, and a vector in others. If this is the case here, the student would perhaps be better advised to write a container which includes a map and a vector as elements, and provides those operations on map and vector that are needed.

If this was intended as a vector, not a map, then changing the code to begin: class Reports : public vector<Report> removes the need for GetReport(); vector provides operator[] to retrieve items.

GetReport() is a mess: it permits nReport to be negative, but does not handle this case (nReport should probably be unsigned, not int). It also relies on the fact that map is a sorted associative container to ensure that a report's number does not change regularly; since there are no guarantees about the values of the keys used, this could lead to unexpected behaviour. Finally, it uses casts to trick the compiler into returning a bad reference. The comment implies that this was done to silence a compiler warning; in this case, the warning is generated since GetReport() can fail to return a report. However, vector's operator[] will handle this case for us.

Finally, a couple of style notes: (*iter). is better written as iter->, and preincrement is generally preferred to postincrement, as the code is slightly simpler (although the optimizer should fix this for you).

Putting this all together results in the following code:

// An exception thrown whenever a requested
// report cannot be found

class NoSuchReport {
    private:
    unsigned report_number;

    public:
    NoSuchReport(unsigned nReport) : 
      report_number(nReport) {}
    unsigned getReport()
    { return report_number; }
    // Compiler can safely autogenerate the 
    // big four.
};

// Reports : vector of reports
class Reports : public vector<Report>
{
    void ClearAll()
    {
        for(iterator iter = begin();
            iter != end(); ++iter)
        {
            iter->second.ClearAll();
        }
    }
};

Should GetReport be needed for legacy reasons, it is easy enough to implement as follows:

Report &GetReport(unsigned nReport)
{
    return (*this)[nReport];
}

This code looks relatively innocuous on a first reading, but repays further thought as there are many problems lurking within this relatively short piece of code.

To my mind the two most serious problems are (1) the confusion about whether this class contains a map or a vector and (2) ownership issues.

The first criticism concerns access to the objects of the class. Does Reports represent an array or an associative container? It may be both, but there is a fundamental asymmetry about the class as currently coded since all the associative behaviour comes form inheritance and the array like behaviour by an additional method. My inclination is to suspect the inheritance from map and I would prefer the class to contain a map as member data and provide methods to manipulate it.

There are several ownership issues. Firstly, the default copy constructor and copy assignment operator make it possible to copy the data structure. This is probably not behaviour the writer expected and is likely to be expensive; as the map contains the reports each one will be copied. There are two standard solutions to this particular issue: either provide a private definition of these two methods (but no implementation), or inherit from a non-copyable class such as boost::noncopyable.

The second ownership issue concerns the member data nIndex and iter. These members seem to be designed to optimise iteration over the elements of the class by keeping state between successive calls to GetReport but this is unreliable if the collection changes between calls to this function. I am also unhappy that member data like this is made protected rather than private. If the performance of the iteration actually is an issue (as determined by some performance measurements) then a better solution would be to provide an array-like iterator for the class following the usual STL pattern.

The third question I have about ownership is the onus seeming to be on the user of this class to call ClearAll() before the Reports object is destroyed. I would prefer this to be linked to the class's destructor for safety.

The final comment I would wish to raise with the writer of the code at this point would over the return of a null reference when GetReport fails. References never should be null, and returning one makes the code non-portable and well as unreliable. It would probably be better to indicate failure by throwing an exception, perhaps std::out_of_range, rather than twisting the language simply to remove the compiler warning.

The editor's choice is:

Simon Farnsworth

Please email <francis@robinton.demon.co.uk> to arrange for your prize.