Title: Student Code Critique Competition 6

"Why did you implement BookList like that?" complained Tim Leader. "I can add Books and ChildBooks to the list, but I can only remove them as BookBases that have no methods. What use is that?"

"I know, it's ridiculous", replied Nathan, "but you stated in the design model that it's supposed to follow the Composite design pattern."

"Oh take no notice of that," said Tim Leader. "Des Authority read a book on patterns and then mandated that every class in the model had to have an attribute mapping it to a pattern. It's not appropriate in most cases, but you can't tell him that so I just fill the attributes in randomly."

Relieved, Nathan deleted the BookBase class and made BookList a typedef for list<Book>. Just to be on the safe side, he thought he would write some unit tests. Tim's view is that if he "gets the design right" implementation is trivial, so unit testing is unnecessary, and anyway there isn't time for it. Nathan isn't convinced. His tests showed that he could get items back from the list as Books, with titles, authors and prices unchanged from when they were added to the list. How reassuring. Maybe he could get some mileage from a few more tests. He checked the default constructor for Book. title() and author() returned null pointers, not the null string. Whoops! He had used single quotes instead of double in the constructor member initialisers. Obviously this was a typing slip as he hadn't done the same for ChildBook. He noticed that he could also delete the existing default constructor for Book by adding default values to the other constructor. For tidiness he used "" rather than "\0" for the null string. After this fix, everything went fine until he had the idea to try to assign a Book object to itself. Access violation! Null pointer! He added

if(this != &your_book) { ... }

to the assignment operators to prevent this [if he ever gets to go on one of my courses, he will learn a better, exception safe, idiom. Francis]. While he was about it, in deleteStuff() he added checks that the pointers were not null before attempting to delete them and set them to 0 after deletion. Now everything ran smoothly. [It was several months latter that he discovered that delete expressions already check for null pointers. F]

A few days later, Nathan was bored. Tim Leader was "gathering requirements" and after that he would be "getting the design right". It would be weeks before he had any work to pass on to Nathan, so Nathan settled down to studying Stroustrup's 3rd Edition in the hope he would discover ways of further improving his code. He started with const; the access functions didn't look quite right. Clearly returning const char * for title() and author() is correct since the internal members are being returned and clients must be prevented from modifying their values. But the other const (even if it had been the correct side of the *) serves no purpose. What was really needed was for the functions themselves to be qualified by const, since they do not modify member data. Likewise price() and operator ==() should be const functions. price() returns a double by value, not by address, so it is unnecessary for its return type to be qualified by const.

What about the modification functions? Nathan had dutifully followed Tim's design specification, but surely a book's title and author are intrinsic to its identity, and so should not be modifiable? He decided to delete them, and renamed price() to set_price() so that it would not be confused with the access function by human readers.

He found a neater way of implementing the data manipulation functions. He made them private and non-virtual in Book. Then in ChildBook he deleted the corresponding functions and used the assignment from Book in the assignment for ChildBook:

if(this != &your_book) {
    Book::operator=(your_book);
    my_reading_age = your_book.my_reading_age; }
return *this;

He used the same idea in operator==(); this meant that he could make all the data members private, too, improving the encapsulation of the classes.

The code common to the Book constructor and copyStuff() was factored out into another private member function:

void copyStuff(const char *title, 
       const char *author, double price);

In looking more carefully at this code, he discovered that the standard requires his calls of new to be qualified by (std::nothrow) if he is to use them this way i.e. checking whether a null pointer is returned. However his compiler didn't support this qualification but the code did work as he had written it.

Having improved the implementation, Nathan looked at the bigger picture. He wrapped his classes in namespace publication, and replaced the friend operator<<() member functions with functions like:

ostream &output(ostream &);

Then, outside the classes but within the namespace he declared the operator<<() functions and implemented them by calling the respective output() member functions. Access to his classes was no longer compromised by their friends.

Finally he reinstated BookBase as an abstract class, declaring title(), author(), price() and set_price() as pure virtual functions. He derived Book from BookBase and changed BookList to list<BookBase *>. At a stroke he had reduced the dependency of clients upon concrete classes in his namespace, and hence reduced the impact upon clients of any future changes made to those concrete classes.

In the abstract BookBase class he also declared a virtual destructor with an empty implementation (not pure). This eliminated the major bug that had so far escaped his attention: a ChildBook can be added to list<Book> but when that list is destroyed the Book destructor is called, not the ChildBook destructor. Even if BookList is declared as list<Book *> the Book destructor is called for ChildBooks, because the destructor in Book is not declared virtual. No harm had come from this omission yet because no resources are allocated in the ChildBook constructor over and above those allocated by the Book constructor. But having ridden his luck this far, Nathan did the decent thing and made the destructors in Book and ChildBook virtual (he also made the functions title(), author(), price() and set_price() explicitly virtual for tidiness).

When Tim Leader eventually got back to using Nathan's code he was amazed. At first he had doubts about the namespace, arguing that it was a Microsoft specific feature only really useful for resolving name clashes. He only yielded when Nathan showed him chapter and verse from Stroustrup. He thought the abstract class was wonderful, and set off to enlighten the rest of the project.

One day, Del Iverymanager called Tim Leader into his office. "As you know, Des Authority is leaving us to join yadotcom.com. Since you came up with that great idea of using abstract classes, I think you're the man to take on his role. What about Nathan, do you think he could do your job?"

"No, I don't think so", Tim shook his head, "he's not up to speed with C++, has to keep referring to that book of his."

"I suppose you're right", agreed Del knowingly, "I had noticed that his subsystem has fewer lines of code now than a month ago, and according to my planning tool that means the project has slipped another six weeks."

We start well here, with good intentions at least. In the light of almost all of Overload 39, particularly Kevlin Henney's "Source Cohesion & Decoupling" (Henney2000), we have an include guard, in all capitals, reflecting the name of the file.

Forward declaring a class so that its complete declaration need not be included is laudable, but in the case of template classes (the stream classes are all templates in modern compilers - I note here that Francis already mentioned the use of a pre-standard compiler) it is difficult, if not quite impossible, to provide the complete declaration. The header <iosfwd> is provided for this express purpose, however, and has the same effect.

An empty class can only signify a base class. An empty base class, to my mind, indicates a design flaw; perhaps this is what Francis alludes to. Any classes derived from this empty class can be seen through a pointer to the base class. However, the base class being empty, this pointer would have to be cast to the static type anyway for it to be of any use. Having a common base class indicates further that such a pointer's static type may be variable, in this case it could be a Book, a BookList, or a ChildBook. Actually casting to the static type would involve what is to me a misuse of RTTI information. So much for empty base classes. I will not yet rule out the possibility of a common base class for some of this hierarchy however.

Looking into class Book, then. First up, a constructor with multiple parameters. Two char*s, evidently indicating strings of some kind. These should definitely be const, since they must be copied to data members of the Book class in order to be safe. Looking ahead, these data members exist, and they are indeed copied from these arguments. Using std::string or a similar "value" class would have benefits here, and remove some sources of error. Even then, they should be const references.

There is also a default parameter. This has the effect of making this two constructors in one, although in this case I do not see any real problem with that (it is not my preference). I will return to this point later.

The void parameter for the default constructor and destructor is redundant in both cases. Again, this may be the compiler's fault.

Rather more serious is the default constructor's initialisation list. my_title and my_author are both char*s. char * c('\0') achieves the same result as char * s = 0; this may be the intention, but leads to errors elsewhere. Importantly it does not set the contents of the string to be a single null character.

The destructor for Book should be virtual. Since Book is used as a base class for ChildBook, and more particularly since it requires a defined destructor to free up memory for the strings, the absence of a virtual destructor may result in serious memory leaks.

The copy assignment operator suffers from one anti-idiom: "release before copy." Just swapping the two function calls would not work either. This needs a little expansion.

deleteStuff() is fine. It uses the correct versions of delete[], but, to risk harping on, would not be needed if std::string had been available.

copyStuff() is less secure, and suffers from basically the same problems experienced by the original code. Memory is allocated, data is copied, memory is allocated, data is copied. If the second allocation fails, memory allocated before it is leaked. Again, a pre-standard compiler appears to be indicating memory allocation failures by setting the pointer to 0 instead of throwing an exception. In this case, if the second allocation fails, then Book is left in an inconsistent state.

More seriously, if the Book being passed in to copyStuff() had only been default-constructed (recall that the default constructor set all strings to 0), we have undefined behaviour, as these pointers are dereferenced in the call to strlen().

In order for copy assignment to be made safe in the presence of memory allocation failure, we need to perform the copy before the release (CBR) (Henney1998). By making use of a temporary Book object, we can construct it with the values to be copied, allocate new space, release the old space, and finally copy the temporary values into this space. The simple idiomatic way of doing this is well documented elsewhere, but put simply, assuming an existing swap function for Book types:

const Book& Book::operator=(const Book & rhs){
    swap (*this, Book (rhs));
    return *this;
}

The temporary Book created in the call to swap is destroyed on exit from the function, so the original memory is freed after everything else has been copied successfully. If the swap fails, then this is left in the same state as before the copy assignment was invoked.

Both deleteStuff() and copyStuff() are virtual and protected. The reason for virtual is clever, but unnecessary I think, besides which, the virtual-ness of the functions is never used. Given that both Book and its immediate child class, ChildBook, define copy assignment anyway, the contents of these functions may as well be placed there; the extra level of indirection is unnecessary.

The equality comparison operator should be a const function. Lastly, the use of protected data. The original code suffered from this, and I cannot see a good reason to carry it over. Better folk than I have commented that it generally indicates bad design, and I see no reason to disagree.

Specifically, if Book's data were made private, ChildBook would not have direct access to it. Book, however, provides accessor functions to its data. Whether knowingly or unknowingly, the author correctly made these accessors non-virtual. This represents what Scott Meyers (Meyers1996) calls an "invariant over specialization", something which a derived class can use in the base, but cannot change how it is implemented.

ChildBook, then, does not have these same data members, relying instead on the base class to correctly implement these common data items (author, title and price). However, it implements reading_age, and so should provide copy construction and assignment. With no inherited data, this would take the following form:

ChildBook::ChildBook (const ChildBook & rhs)
    : Book (rhs), reading_age (rhs.reading_age)
      { /* empty constructor */ }

This works because a ChildBook can be implicitly converted to a Book. There is no need for the base class to allow for a converting copy constructor for every base class (a maintenance nightmare) because of this.

Correspondingly, ChildBook has no accessors for data members present only in the base class, but the following will still be valid, calling the base class function:

ChildBook book;
book.name();

We have now removed all inherited items from Book! Ah well, at least we can put one other virtual function in. I have seen this idiom, and used it, but I cannot remember the reference (apologies if you are the author). At the moment, both Book and ChildBook define separate stream output functions. We can factor this so that just one operator<<() calls a virtual function on a Book reference argument. Book defines a (virtual) print() function, which is over-ridden in ChildBook.

class Book {
public:
    virtual void print(ostream & os) const 
            { os << title << author << price; }
};
class ChildBook : public Book {
public:
    virtual void print(ostream & os) const 
        { Book::print(os); os << reading_age; }
};
ostream & operator<<(ostream & os, 
                                        const Book & rhs){
    rhs.print(os);
    return os;
}

This relies on the fact that references, as well as pointers, are polymorphic in their behaviour. It means also that operator<<() does not have to be a friend function. Note: this isn't really an error in the original code; I add it here hoping that any readers not familiar with this will find it useful.

The accessor functions in Book should, of course, be const functions, and the cast to const char const* is unnecessary, as this conversion takes place automatically for the returned pointer. I think that the need for explicit "setter" functions is doubtful, especially since a full constructor and a copy constructor are both provided. After all, few books change either title or author in their lifetime. If they stay, the arguments should be const, and, as with copy assignment, they suffer from release before copy.

I also question the use of a double to store the price of a book. Quite apart from the magnitude available (there may be books that cost that much but I wouldn't be able to buy one of them) there are issues of accuracy and comparison. Floating-point values have a certain notoriety about them in terms of accuracy of comparison, such as should these values compare equal?

100.01

100.010000000000001

As it happens, we only need accuracy to two decimal places, and storage in an unsigned would probably make more sense, and be more efficient into the bargain. For output purposes, a simple division by 100 is all that is needed.

The ChildBook class has one other feature that I promised to return to earlier: a constructor with default arguments. In this case all the arguments are defaulted. There are four of them, which means this is a five in one constructor. What is more serious is the fact that, under some circumstances, a char* can be implicitly converted to a ChildBook. Here is one such circumstance:

void some_function (ChildBook b) { }
char * s = "Hello, World";
some_function (s);

This may be contrived, but it illustrates the point. In the absence of keyword explicit, this can be avoided by having separate constructors for each possibility. In the case of Book, the base class, the default constructor created a "default" Book (naturally), and a single constructor for all data types was given. Having made changes already mentioned regarding data members, ChildBook is left with only a single data member anyway, so we can only remove the problem of implicit conversion with a dummy argument:

ChildBook::ChildBook (double your_reading_age, int dummy) : reading_age (your_reading_age) { }

As with Book::price, using a double for reading_age is suspect.

The only other point I would make is for the sake of readability. ChildBook should really define a destructor. As it happens one is not required, and the (now virtual) destructor for Book does all the work, but I believe this might be a source of future error. My preference is to explicitly label it virtual.

BookList suffers from two problems. Firstly it derives from the common base class, BookBase, which indicates that both a BookList and a Book have things in common. In this case, they do not (although they might, if BookList were some kind of index, for example), so a public inheritance relationship is not the right thing here. Secondly, BookList is a container of concrete types. If ChildBooks were added to this list, they would be "sliced", becoming mere Books instead, completely losing the reading_age information. This is because each item added to the list would be copied using Book's copy constructor, which takes a Book reference. In order for the list to contain multiple types, it would have to contain BookBase*s (or Book*s), but this suffers from different problems.

Such a list of BookBases makes no distinction between a BookBase, Book, ChildBook and BookList. This list could contain a mixture of these three types of "thing", with no way of distinguishing the concrete type of each without resorting to RTTI (which itself relies on polymorphism, which is missing from Bookbase) hackery, because BookBase itself allows no operations and has no data. Even if the list were of Book type, allowing Books and ChildBooks, a cast would have to be made from Book to ChildBook to access the reading_age of the item, but if the item were not a ChildBook at all, merely a Book, the conversion would fail.

If the collection were to be treated as if all of them were Books, we would not have a problem here; the list might only be used to provide the titles and authors in the collection. This use would be perfectly valid in that case, although not necessarily moral.

Making the operator<<() a friend is superfluous in this case, as the list data member is already public. More to the point, since there is only one public data item, and no member functions, the need for BookList at all is superfluous, and the same effect could be achieved with a typedef.

I think that the relationship between Book and ChildBook is valid, and useful. On reflection, a common base class for them is not needed, and, even as a concrete type, Book makes a sensible base class, reflecting as it does the common elements needed for ChildBooks and maybe ComicBooks, TextBooks, and so on.

As already commented by Francis, a compiler with no string value type is poor indeed. For the purposes to which char* is put in this code, it would be worth composing a simple string class to handle lifetime and assignment issues. I think an outline for such a class has appeared in 90% of the C++ text books I have read (good and bad) so I'll not outline one here.

Finally, printing details of each book in the list. The for_each thingy you mention goes something like this:

typedef list<Book> BookList;
struct print {
    print (ostream & os) : strm (os) { }
    void operator() (const Book & rhs) 
                                        { rhs.print(strm); }
    ostream & strm;
};
ostream & operator<< (ostream & os, const BookList & b) {
    for_each (b.begin(), b.end(), print (cout));
    return os;
}

The print structure is called a function object, or functor. Briefly this is so due to the operator() member function. For a fuller description, check Stroustrup's "C++ Programming Language" (Stroustrup2000).

I sincerely hope this is useful to you, and other readers. All the best with your degree.

[The complete article and code is available on our web site. FG]