Title: Student Code Critique Competition 5

I shall work through the example in the order printed. It is clearly a fragment, since there is no inclusion of <list>, which is required for compilation.

class Book

Well, OK. One could argue that the class is merely a means of identifying a book, rather than representing a whole book. The name "Book" might imply rather more, like the complete contents, layout, page numbering etc.

virtual void details();

What does this function do? It is virtual, so derived classes are expected to override it, but there being no description it is difficult to know what ought to happen. Since the function takes no parameters and returns nothing it probably does something to its data members (well, it isn't const, so it is allowed to change its own state). Maybe it uses some globals, like printing to standard output. If so, passing in the data it needs would be far clearer and allow it to write to other files, e.g.:

virtual void details(ostream&);
void details(char*, char*, float);

Now one thing that strikes me is whether the author thought that using the same name as the previous routine somehow endowed the virtualness onto this routine, and the details routine does not exist at all. This is not how C++ works, if two functions declared in a class have different arguments, then they are different functions, and virtualness does not transfer to similarly named functions.

What about that comment? We can embed the names of parameters in the prototype, which obviates the old C-style necessity of making them comments, so the prototype ought to be:

void details(char *Author, char *Title, 
                      float Pages);

[actually, those char* parameters almost certainly should be of type char const* FG]

How many pages are we expecting? So many that the number won't fit into 32 bits? I hardly think so. You would have a book several kilometres wide with that number of pages. Peeking ahead, I see that the data stored is "price" not "pages", so is the comment out-of-date? A very frequent occurrence, and the problem with comments that aren't really part of the code. I would also be worried that since the first two parameters are both the same type, they may be the other way round from that stated, since this is the way the data is stored in the class.

Why do we want to add the details of the book after it is created? Surely the only thing that might change is the price, just possibly the title, but unlikely all together. We probably need a function:

void SetPrice(float price);

which would just change the price of the book.

virtual void anzeigen();

I am reliably informed that this could be translated as "show". So maybe this is the routine that displays the data. The same comments apply to this as to details above.

Book();

Unless we are in the business of creating a book from scratch, I cannot see the requirement for this constructor. It should probably be private and not be implemented.

~Book();

Now we have already established that this class is to be used as a base, by the existence of virtual functions, so the destructor really ought to be virtual. It is extremely rare that you can guarantee that the destructor will always be called at the right level of hierarchy, and if a derived class is deleted though the base type then problems will occur. If the compiler knows that it can bypass the virtual table, then it will. Normally it will not, so making the destructor virtual ensures that the correct function is called.

Book(char*, char*, float);

Once again, there are no parameter names, so I cannot be sure which way around the first two arguments are supposed to be provided. Shouldn't the arguments be const?

Book(const Book&);
Book & operator= (const Book&);

No problems with these

bool operator == (const Book&);

Since I assume that this only compares two Book objects, why is it not a const function?

bool operator == (const Book &book) const;

This would prevent the accidental writing of:

return price = book.price;

which, far from comparing the prices, would overwrite the current price with that being compared.

protected:
  struct book {
  char *title;
  char *author;
  float price; } b;

Why is the data protected, not private? Unless there is a very good reason, data should be private, and derived classes should use the public or protected functional access to that data. This ensures a very restricted set of routines that can alter the data; making debugging easier.

Why is the data being hidden in a struct? The class itself is a structure, and there is no reason to add a further level of hierarchy. In fact, adding this level prevents constructor initialisers from being used, unless this structure is given its own constructor, which starts to look very silly.

Why are you using char*? Use the STL type string and you will produce far fewer errors in your code.

I do not think that the price should be a float (assuming that it is the price, not the number of pages). Holding the price as a fixed-point value (i.e. in whole pennies, pfennigs, cents, etc) ensures that price comparisons can be made without regard to strange floating-point effects. You might want to wrap a class round it so that you can determine centrally how to print the price, or provide currency conversions.

class ChildBook : public Book

Seems perfectly reasonable.

public:
   int reading_age;

Why is this piece of data public?

void details(char*, char*, float, int);

This looks like a copy-and-paste job from the base class. The comment clearly does not match the function prototype, since there are now four arguments but only three comments.

The rest of the functions in this class are the same as those in the base, and so the comments for the base apply here too.

class BookList : public Book

Is the BookList a Book in its own right? Well, it could be, since one's library might contain an index to all the books in it. I shall leave this be for the moment.

  Book b;
  ChildBook k;

So this book contains two other books? It does not look right, but given no example of its use, I cannot be certain that it is wrong.

list<Book> Book;

Now my compiler warns me that I am reusing the identifier Book, originally a class name, and now as a variable name. With the implementation as it stands, the file will still compile, but warnings should be heeded because it might indicate problems to come. If you use an initialiser in the constructors of this class, then the base class is called Book, and so is this member, so the compiler will not be able to distinguish between the two.

list<ChildBook> kiBook;

You have provided a separate list for ChildBooks. What happens when you want to derive further types from Book? Do you need a list for each derived type? That does not sound very useful. Would it not be better to have a single list of pointer types (possibly smart pointers) so that any book type could be added to the list? At the moment you cannot add a BookList object to the list without losing information, since the object stored would only be the base Book, not the full BookList type.

If that does not make sense ask this question: Do you really have a book (the BookList) that contains many other books? I suspect that either the derivation from Book is wrong (so BookList is not itself a Book), or your BookList ought to contain a list of pointers to books.

  void details();
  void anzeigen();

No comment.

  ~BookList();
  BookList();

Where are the copy constructor and assignment operators? They need to be provided in the class, but possibly not implemented.

bue.Book.push_back(Book("Book", "Title", 20));
  bue.kiBook.push_back(ChildBook("Author",
         "Title", 35, 5));

So the order of arguments for the constructor is author, title is it?

What do these lines do? Firstly, create a Book/ChildBook object with the appropriate constructor. This is a temporary on the stack. Then it copies that object into the appropriate list. Note that the created object is not put into the list. Why not? Because if it were, at the end of the statement the temporary is deleted and the list becomes inconsistent. All STL containers take possession of their contents by copying them to ensure that users cannot produce inconsistencies in this way (although it cannot guarantee safety if you use raw pointers).

Why is there no "add book to list" function in BookList? Why are all the members public? Surely to save work when you (inevitably) decide to change things, you should have a function to conceal the implementation of the BookList:

void addBook(const Book&);

If you follow my advice earlier, then you need to pass a pointer to the Book, so this would become:

void addBook(const Book*);

and your function calls above would be:

bue.addBook(new Book("Book", "Title", 20));
  bue.addBook(new ChildBook("Author",
                        "Title", 35, 5));

whether the addition of ChildBook and Book do different things can be decided inside BookList, rather than hard-wiring it externally.

Book::Book()

I have already stated that this function is probably redundant. It certainly should not fill in arbitrary data.

Book::Book(char *t, char *a, float p)

Aha! So the parameter order is title, author. We could be in for some surprises here.

b.author = new char[strlen(a) + 1];
   strcpy(b.author, a);

Now there is a library function to do this:

b.author = strdup(a);

[Colin is mistaken here, many unix systems provide such a function for C, but it is not a standard function, not least because who is going to do the cleanup. FG]

That saves the potential problem of allocating the incorrect amount of space, by forgetting the +1 in strlen, for example. This means that you should be able to do all the work in the initialisers, e.g.:

  Book::Book(char *t, char *a, float p)
      : title(strdup(t)), author(strdup(a))
                      , price(p)
  {}

However, with your nested struct, this cannot be done, since title etc. are not in the direct scope of Book.

Had you used strings, then the constructor would be even simpler:

Book::Book(const string &t, 
          const string &a, float p)
  : title(t), author(a), price(p)
  Book::Book(const Book &p)

The same comments apply to this function as to the previous one. There is no reason to have anything the in the body of the constructor.

Book::~Book(){
  delete b.title;
  delete b.author;
}

I am glad you knew to delete these data members. Had you used string types, then the destructor would be empty - mush simpler to write.

[and he would have avoided the trap of using the wrong form of delete. FG]

  ChildBook::ChildBook(char *t, char *a, 
                  float p, int la)
  : Book(t,a,p)

Why haven't you used the initialiser notation for reading_age? I know it does little for you while reading_age is an integer, but suppose it is changed at some stage to be some other type that looks like an integer but does more? For example a type that checks that the value is within reasonable bounds? Use the initialisers and then you are likely to have less to change when you improve the types.

  ChildBook::ChildBook(const ChildBook &k){
    b = k.b;
    reading_age = k.reading_age;
  }

Now here is the crunch. There is no excuse for not using the parent class's constructor from the child's. In fact, it is rarely right to omit the parent class's constructors. As it stands, the Book's default constructor is used, creating useless space and wasting time, and then you are trying to copy the ChildBook's data into it. What does the line b = k.b do? It copies the bit pattern of k.b into b. It does not duplicate any strings and you lose the space allocated in the Book constructor. When the argument k is deleted, the pointers in the newly-copied ChildBook will continue to point to those in k and will thus be pointing to freed space - and there is your original problem. Had you made the data in Book private, or removed the default Book constructor, then you would have been forced to use the Book's copy constructor and these mistakes would never have occurred.

However you implemented Book, the ChildBook copy constructor ought to be:

ChildBook::ChildBook(const ChildBook &k)
  : Book(k), reading_age(k.reading_age) {}

Finally:

BookList::BookList(){}

Why are none of the data members initialised? You have a base-class Book that needs a constructor, and those members b & k.

Thanks, Colin, for a comprehensive critique. Curiously, while you have identified many of the faults with the code, you have missed the cause of the destructor call that was puzzling the student. FG

"What do you make of this, Holmes?" On our return from the Diogenes Club where we had dined with Mycroft Holmes, brother of my esteemed companion, Sherlock, I discovered upon our doorstep an envelope addressed to "Mr. S. Holmes And Dr. J. H. Watson".

"Open it, then Watson! Let us hear it!" he cried with vigour. "After such a dull week as this, I shall be happy to look into any small problem our mystery correspondent is troubled with."

I sat at the bureau, and slit open the envelope. Without being asked, I passed the envelope to Holmes, and quickly read through the letter it had contained.

"There is nothing of interest on the envelope. What of the contents?" Holmes asked.

"It appears to be some kind of code," I replied, passing the letter over. "I'm afraid I can make neither head nor tail of it."

Holmes peered at me mischievously through his eyebrows. "Very well, then. We'll see if I can do any better."

As soon as he looked at the page, his eyes lit up. "Ah hah! A real rarity. It has been a long time since some poor soul has looked to me for their programming problems. C++ code, I see."

"You've heard of this code, then Holmes?"

"Hmm. I wrote one or two short monographs on the subject when I was younger. You might care to look them up, if the subject is to your interest." He returned his concentration to the page, while I retreived the papers he mentioned, and quickly scanned through the text for clues as to how my companion might continue.

The facts appear to be these. We have three classes, Book, ChildBook and BookList. A book represents an entity with a title, author and price. A ChildBook, now what is that? Ah, I see now. It is a book for children! In addition to the main data for Book, we have a reading age entry. ChildBook is derived from Book, but unsafely, I perceive. Ah well, we'll return to the details momentarily.

"BookList is also publicly derived from Book, and yet has both a Book and a ChildBook data member, and a list of each. Well, definitely a rather serious error in design for us to consider in our diagnosis, Watson."

I'm afraid I was left a little bemused by Holmes' apparent knowledge of the subject, and so asked for an elaboration.

"It is a small matter, but an important one. When publicly deriving a class from another, one must ensure that the derived class satisfies the identity of the base class; one must ensure the derived class IS A base class, in the sense that they can be interchanged. In this case, a ChildBook clearly IS A Book, and so satisfies this condition, but a BookList is not. On the other hand, when privately deriving a class, the derived class is said to be IMPLEMENTED IN TERMS OF the base class, which can often be achieved through aggregation: including a data member of the base class type in the derived class itself. Unfortunately, BookList doesn't exhibit an IS A relationship with Book, but it is IMPLEMENTED IN TERMS OF Book. From what I can see, it is merely a collection of Books and ChildBooks, and thus needs no inheritance relationship."

My frantic cribbing of Holmes' notes gave me a basic understanding of the terminology. I nodded for him to continue.

"Now we get to the crux of the matter. Our friend is having difficulty populating his BookList entries with new Books and ChildBooks. Well, no surprise so far. What is puzzling is the apparent execution of the ChildBook destructor immediately after insertion into the list. Perhaps these function bodies will throw some light on the subject.

"Firstly, the creation of a Book. We allocate memory space for the author, and then initialise it. Then, we allocate more memory for the title, and initialise that. Finally, we set the price. Well, there is one serious problem here, but it probably has no bearing on the immediate problem."

"How is that, Holmes?" As ever, my companion's insight astonished me.

"Well, if the second memory allocation fails, probably throwing a bad_alloc exception, then the memory allocated for author is leaked. I notice exactly the same problem in the converting constructor and copy constructor."

"Good heavens, Holmes! Brilliant!"

"Elementary, my good friend. And yet, there is more. Do you notice that the memory is allocated for an array of characters, using the new[] construct? Our friend has correctly deduced that the memory allocated in the constructor needs to be de-allocated in the destructor for the class, but has used only the basic, single object version of delete. For de-allocation to be performed successfully, the array version of delete is required (for example, delete[] b.title). This I perceive to be at the heart of the problem. Still, there is yet a better solution to this, which doesn't require delete at all. Would you care to speculate on that, Watson?"

"Well, I think that we could replace the character arrays with std::string, thereby instilling intrinsic data type semantics to the data members, precluding the need for a destructor altogether."

"Excellent, Watson! I see you've been studying my methods more closely than I give you credit for. As a matter of style, I would put the initialisation of the data members in the initialisation list of the class. In its current state, this cannot be done, since initialising the members requires a function call. Replacing the char arrays with std::string, however, means that the copy constructor, for example, can become:

Book::Book (const Book & p)
  : author (p.author),  title (p.title), 
    price (p.price) { }

As I recall, only constructors are invoked for non-intrinsic items in an initialisation list, and the compiler can often elide the requirement for a function call in these cases. Thus, we have introduced efficiency as well as style. You will notice that he has used this method for invoking the base class constructor in the converting constructor of ChildBook, but still neglected to take advantage of it for reading_age."

"I am still puzzled by your earlier reference to unsafe inheritance, Holmes. I understood your reasoning completely about BookList not deriving from Book, but the reasons for ChildBook's derivation being unsafe eludes me."

"It is more than the inheritance which is unsafe, Watson. A closer look at the code reveals some rather nasty surprises, which lead me to conclude that my earlier assertion at a simple solution was in grave error!" He reached up to the mantelshelf for his tobacco slipper, filled the largest of his pipes, and lay back, eyes closed, in a brown study. Presently, his eyes snapped open, and he looked straight at me, pronouncing "You might care to get some rest, old friend. This is fully a three pipe problem, but rest assured I will wake you if there are any interesting developments."

I arose at my usual late hour, to find Holmes at the table by the window, with tea and toast laid out before him. "I've opened the window for your comfort, Watson. I must say it did get rather stuffy in here during the night!"

Indeed I could still detect the lingering odour of Holmes' rather pungent tobacco, and was rather glad he had let some fresh air into the room.

"We have tea, juice and some toast is left. I'm sure Mrs Hudson will oblige if you require anything further."

"But what of the problem with which you spent the night? Have you solved it?"

"Ah, yes. Well, calm yourself, dear Watson. You'll not be needing your trusty service revolver for the conclusion to this little mystery! I know I promised to wake you, but I'm afraid I couldn't find the courage to do so.

"So, the facts as we know them are these: we have a class Book, which suffers from exception unsafety in its memory allocation, and undefined behaviour in its use of the incorrect version of delete in its destructor. We have a class BookList, which inherits incorrectly from Book, and a class ChildBook which inherits, apparently correctly, from Book.

"We have already theorised on a solution to both the exception safety issue, and the incorrect use of delete, by replacing character arrays with std::string.

"In the course of the night, I discovered several more serious issues with our code, which I will list now:

"No. 1. Class Book is evidently used as a base class (as indicated by its having virtual functions) yet the destructor is not virtual. It was to this I alluded when I made the comment about unsafe inheritance. Note here that a destructor needs to be virtual only when a base class pointer will be used to delete a derived class object. In this case, an example of this might be:

Book * b = new ChildBook ();
delete b;

If the destructor for Book is not virtual, then b will probably not be destructed properly. In our case, I see it is not too serious, as ChildBook has no defined destructor, and only one data member which is of an intrinsic type.

"No. 2. The arguments to the details() function and converting constructor for both Book and ChildBook should be const. Under the circumstances, this could arguably be described as a style issue, but it would prevent the following:

std::string name ("...");
std::string title ("...");
Book (name.c_str(), title.c_str(), 10);

This is because the c_str() member of std::string returns a const char array, and a const object cannot be converted to a non-const one. You will notice that where references are used, our friend correctly makes them const. So, at best he is inconsistent in his use of const.

"No. 3. This is the most serious error of logic in the code. In the constructor and copy constructor for class Book, our friend is careful to allocate memory for the data items in the book structure (I'll come to naming conventions shortly), and perform a proper deep copy of the pointers therein. However, he neglects to do this for the copy constructor of class ChildBook. It is important to remember that, for non-simple types such as classes or structs, the default copy constructor, as provided by your compiler if you do not, performs only a shallow copy; in the case of pointers, only the addresses are copied, not the content of those addresses. The copy constructor for ChildBook contains the following line:

b=k.b;

Where, in both cases, b is of type Book::book, a struct containing two char pointers. Since Book::book does not declare, in this case, an assignment operator, the default is used which performs a shallow copy of the pointers.

Now consider this line of code:

bue.kiBook.push_back (ChildBook("Author","Title",35,5));

kiBook is a std::list of ChildBooks. A temporary ChildBook object is created as a parameter to push_back(). In its constructor, memory is allocated for copies of the text items, "Author" and "Title". This object is then copied to a new object in the list (remember that list stores copies of objects, not references to them), but only a shallow copy of the ChildBook::book members (which it inherits from Book because they are protected data - more on that in a moment). When the temporary object goes out of scope, its destructor is invoked, as experienced by our friend here. This has the effect of deleting the memory for those data items. So now, the object stored in the list points to deleted data. This, coupled with the fact that the incorrect version of delete was used on those members anyway, causes undefined behaviour in the program, and is the reason why our friend is experiencing problems."

During this discourse we had retired to the sitting room, and the comfort of the lounge chairs. As Mrs Hudson cleared away the remains of our breakfast, my illustrious companion completed his analysis of the C++ Code Problem.

"So there you have it Watson. In resolving the problems, we can return to our earlier hypothesis regarding exception safety in the Book class. I recall it was you who suggested using std::string to alleviate exception safety issues. If we look at the issues we have since uncovered, we discover that replacing our char arrays with std::string also removes the need for delete-ing the memory in the destructor for class Book (and, by the way, removing the use of the incorrect version of it), and more importantly, though related, removes the need for deep copying in the constructors of each class, most important of course in the copy constructor of ChildBook.

"It is this last point which will cause all of our friend's problems to vanish!"

"That's wonderful, Holmes! My word, it seemed to me to be such an impenetrable problem, and yet is solved in such a simple fashion!"

"Quite so, Watson. It is often the case that when looking into some difficulty, one overlooks the most obvious solutions. As you know, my methods are a useful discipline against such errors, yet I still make them from time to time.

"Still, there are one or two points on which I would like to alight if I may. Firstly, the issue of inheritance. The destructor for class Book should be virtual, since it makes sense that ChildBook inherits from Book, it should do so safely. It seems to me that having the data members for Book in their own struct is pointless, and so see no reason not to make them simple data members, and make them private. In addition, of course, we make the char arrays into std::strings.

This brings up the issue of the details (...) function. This is not virtual, evidently because the function signatures differ; were the Book::details (...) function to be virtual, the ChildBook::details(...) function would hide it, not override it anyway. However, it is enough that ChildBook::details (...) calls Book::details (...) as the first call in its body:

ChildBook::details (const string & t, 
                const string & a, 
                float p, int r)
{
  details (t, a, p);
  reading_age = r;
}

"So the declaration for ChildBook should contain the line:

using Book::details;

"and Book::details (...) should not be virtual. Note that the line using Book::details; brings the name details into the scope of ChildBook; it allows the function to be overloaded in the usual sense. Base class public members are still accessible in the usual way. Since the function signatures differ, we could not use a pointer to Book to access ChildBook::details (...) anyway.

"From this it is easy to see that the virtual-ness of Book::details() (with no arguments) is still valid. The name has been brought into the scope of ChildBook, so no hiding takes place where arguments differ, thus allowing overloading in the usual sense. However, details() itself still has a place in the virtual function table, and so ChildBook::details() can be called via a Book pointer. So, details() should be virtual in the base class, but details (...) should be virtual in neither class, since ChildBook overloads it. It seems obvious to me that details() is a function which restores the default values, but that is pure conjecture on my part, as no function bodies are provided.

"As already mentioned, BookList should not inherit at all from Book. I also do not see the need for specific data members of type Book and ChildBook; since BookList is a collection of these, the list containers are sufficient.

"As data members, they should also be private, with suitable accessor functions. Our friend has correctly separated the container of Books from the container of ChildBooks. I mentioned before that a ChildBook IS A Book, as indicated by the inheritance relationship. It does not follow, however, that a collection of Books IS A collection of ChildBooks. A secondary issue here is that concrete types (those which may instantiate objects) do not make good base classes; a better solution may have been an interface class, from which both Book and ChildBook inherit.

"Finally, a point upon which I stumbled at first, the use of the identifier Book in the following line:

list<Book> Book;

It is used as both a type and an object identifier. This seems valid, but questionable. The compiler is under no constraints to preserve object names, and so may be able to distinguish easily between which is a type and which an object. Human readers on the other hand may be less fortunate, and confusion can easily arise from such naming conventions. Similarly, before our proposed alterations to the Book class, it contained a struct member, book. Since C++ is a case-sensitive language, Book::book is a valid type, although Book::Book is not (it denotes a constructor for Book). I do not consider this a suitable name for this member, but the point is moot, as we have already removed it.

"And I believe that is all we can deduce from this code fragment."

With that he lit his pipe and leaned back in his chair, puffing smoke rings at the ceiling. Presently he looked up at me from his reverie, intoning: "It means 'display', Watson."

"I beg your pardon, Holmes?" I asked, incredulous.

"Anzeigen, Watson. The German word over which you were struggling," he replied, smiling happily.

"By the deuce, Holmes! How did you know..."

"If I've told you once, Watson, I've told you a hundred times: when you have eliminated the impossible, whatever remains, however unlikely, must be the truth!"

I must still have looked completely dumbfounded, as he then added "Besides, I could hear you whispering it to yourself."

Here is a plausible solution:

class Book {
public:
  Book();
  virtual ~Book();
  Book (const char *, const char *, float);
  Book (const Book &);
  virtual void details();
  void details (const char *, const char *, float);
  Book& operator=(const Book&);
  bool operator== (const Book&);
  virtual string Title()const{ return title; }
  virtual string Author()const{return author;}
  virtual float Price()const{ return price; }
private:
  string title;
  string author;
  float price;
};
class ChildBook : public Book {
public:
  ChildBook();
  ChildBook(const char*, const char*, 
                      float, int);
  ChildBook (const ChildBook&);
  virtual ~ChildBook();
  using Book::details;
  void details (const char *, const char *,
                       float, int);
  virtual void details();
  virtual int ReadingAge() const 
                { return reading_age; }
  ChildBook& operator= (const ChildBook &);
  bool operator== (const ChildBook &);
private:
  int reading_age;
};
// for the purposes of the task at hand,
// I've left the data members public
class BookList {
public:
  list<Book> book_library;
  list<ChildBook> childbook_library;
};

Book::Book() : 
author("Author"),title("Title"),price(0.0f){}

Book::Book(const char*t,const char*a,float p)
: author(a),title(t),price(p) { }

Book::Book(const Book& p) : author(p.author),
       title(p.title), price(p.price) { }
Book::~Book() { }

void Book::details (const char*t, const char*a,float p) {
  author = a;
  title = t;
  price = p;
}

void Book::details() {
  author = "Author";
  title = "Title";
  price = 0.0f;
}

ChildBook::ChildBook() : reading_age(3) { }
ChildBook::~ChildBook() { }

ChildBook::ChildBook(const char* t, 
      const char* a, float p, int la) 
: Book (t, a, p) { reading_age = la; }

void ChildBook::details(const char* t, 
      const char* a, float p, int la) {
  details (t, a, p);
  reading_age = la;
}

// pure speculation here
void ChildBook::details () {
  Book::details();
  reading_age = 0;
}
ChildBook::ChildBook (const ChildBook & k)
  : Book (k), reading_age (k.reading_age) { }

#pragma argsused
int main(int argc, char* argv[]) {
  BookList bue;
  bue.book_library.push_back 
              (Book("Book","Title",20));
  bue.childbook_library.push_back
       (ChildBook("Author","Title",35, 5));
  for(list<Book>::const_iterator 
        i=bue.book_library.begin(); 
      i!=bue.book_library.end();++i) {
    cout   << i->Title() << i->Author() 
        << i->Price() << endl;
  }
  for(list<ChildBook>::const_iterator
        i=bue.childbook_library.begin();
      i!=bue.childbook_library.end();++i){
    cout << i->Title() << i->Author()
        << i->Price() << i->ReadingAge() 
        << endl;
  }
  cin.get();
  return 0;
}

I cannot find the problem the student specifies, my step through shows construction/destruction in the right order, as far as I know (Is this not the compilers job to decide when to dump things?). However I do see major problems with this code, speaking as a self taught student who will be studying for a degree in Computer Systems Engineering by the time this is published I can hardly be considered to have great knowledge of idiom, however I do recognise poor practise. The choice of the student (I presume the design is the student's - if it is the lecturers, I am horrified) to encapsulate the Book class's private data in a struct seems very strange - what possible bonus could this be?

The derived class ChildBook is a logical extension, but why is reading_age a public member? Surely, an access function would be much more secure. I will assume access functions for other members were cut for space reasons, if they were not available the classes public interface is severely lacking, you cannot assume output will only be required in one form and that the data will never change.

Something the student has not addressed is the variable names chosen (or not), in the header file a comment has to be used to explain the parameters passed to the constructor where none is necessary, the identifiers could always be changed to your_title to avoid ambiguity in the definition of the function, alternatively this could be utilised.

The 'library' class is, in my opinion, ridiculous. I do not know much about patterns or how a class hierarchy should be stored in an encapsulating class but I do not believe BookList should be derived from Book, it makes for confusing reading. If it is a requirment to be able to pass a Book or a BookList equally easily then another class, say BookBase should be defined as a common base.

I believe I have addressed most of these concerns in my own version, which uses four classes: BookBase, Book, ChildBook and BookList. BookBase serves as a featureless base class, but has the advantage you can store a BookList in a BookList in a BookList... ad infinitum should you want to do that. My own classes have member functions for both reading and writing the class state as well as overridden operators for assignment, equality checks and output to a stream. I think my code is of a reasonable quality; however two things I feel are lacking

Any comments on my solution will be greatly appreciated. [see below for Nathan's code]

Editorial Comment

Compilers actually have very little choice over where a destructor is to be called. I might have written none, but there is a tiny amount of room for variance in that a compiler can sometimes optimize away a construction and obviate the need for calling a destructor.

If you are using a compiler that does not know about string, then you have a very poor library. String has been a standard library provided type for almost as long as C++ has existed. There have been considerable changes to the way it is provided, but that is a different issue entirely.

I think your idea of having BookBase so that types of books and collections of books can be treated together is interesting. Perhaps some expert on design would write an article on this, highlighting both the strengths and weaknesses of such an idea.

Finally, best wishes on the degree course.