Title: Student Code Critique Competition

From top to bottom, rather than in order of severity then.

Header Guards. As a general rule, do not start any names with underscore. Such ID's are reserved by the compiler.

Header names. On the assumption you are using a modern, reasonably C++ Standard compliant compiler, the new header names should be used:

<cassert>
<iostream>

This will also necessitate stating the std:: namespace in places where cout, endl and assert are used.

Protected data. I prefer to avoid this whenever possible. Having non-private data means at least derived classes can directly access the data, which breaks your encapsulation. In this case, however, it seems entirely redundant, since the class cannot be safely inherited from without a virtual destructor. Even in the presence of inheritance, you have accessor functions which could be made virtual, thus safely providing access.

Constructor with default arguments. I would prefer to see a "proper" default constructor, which uses the initialisation list to default the member variables, and a separate two-argument constructor with no default parameters. I will return to this point later.

Uses of const. In some places it is used, in other places (where it should be) it is not. Where it is used, it appears to be used correctly; it is its absence which should be checked. For instance, operator[] does not change the object in any way, and thus should be a const function.

Uses of assert. When compiled without debug information (i.e. production code) assert does nothing at all. The fact that this is not production code is not an excuse; it is endorsing bad practice. Some uses of assert are acceptable, such as when testing conditions internal to the class during development so that they can be removed, but all of the uses in the Array class are testing run-time conditions which could occur in the "real world." It is safer to use explicit tests (if...else) constructs. assert is commonly (always?) a macro which expands to an if statement anyway, so you are not introducing any code overhead.

From this list, only the issues with assert are potentially fatal. A constructor with any number of default arguments may lead to compile-time ambiguities, and in this case, may lead to unexpected run-time behaviour.

The first thing I looked into was the default template argument. The comment gave me a clue, and I closely followed the use of IType to confirm my suspicion. It is there solely to demonstrate default template arguments. Consider what would happen if a different type were used to specialise Array, e.g. a string:

Array<char, std::string> my_array;

It is not the presence of the two int data members lobound and hibound which make this fail, but their use in conjunction with the template type in the outOfRange() function. If the index type can be parameterised, then both lobound and hibound should be the same type, but this indicates an associative array. In my opinion, its existence is obscuring the code for no good reason.

I want to return to the issue of the multi-argument constructor. It should be understood that this constructor is actually four constructors in one function; it is a default constructor (no arguments), a single argument constructor, and a two argument constructor. The (very) important point that may be missed is that it is also a converting (single argument) constructor. The absence of the explicit keyword here means that a compiler is free to consider this constructor to convert silently from another type, in order to make a call to a function. This can lead to hard-to-spot runtime errors, demonstrated below:

class Bad {
public:
  Bad(const double a = 0): size_(a) { }
  double Size () const { return size_; }
private:
  double size_;
};

class Test {
public:
  void Print (const Array & index) 
          { cout << "Array" << endl; }
// should have implemented a 
// Print (int x),but forgot to
};

int main (){
  Test t;
  t.Print (10);  
// this involves converting to an double, 
// and then constructing a Bad object 
// from the result.
}

This will not only compile, but run, too. It may not, however, be what you expect, especially if class Bad is deeply nested in included files.

At present I am unsure. I am sure the instructor's pep talk and fuller instructions would have enlightened me but it is not clear from the code. I will come back to this question at the end...

One of the best ways (but not the only way) to learn is by example. This code has presented me with an example template class. In the future, if I need to construct something similar, I may refer to this class. I may even reuse the code fragment before hacking it into its new shape.

The following is a list of the unspoken lessons taught by example. These I have judged as being good 4 or bad 8, but please do not assume my judgement to be correct.

// header: header.h

4 document the file

8 but no need to give a full description as the code is self documenting

#ifndef _MYARRAY 
#define _MYARRAY

4 use header guards to prevent multiple inclusions

4 use uppercase for preprocessor defines

8 it is standard practice to not use initial underscores in define names (initial underscores are reserved for libraries and compilers, you may use underscores else where)

#include <assert.h>

8 use the standard C library (this is depreciated, also the use of <cname> is preferred over <name.h>)

#include <iostream.h>

8 C++ library header files need the trailing .h extension (not so!)

4 use <...> instead of "..." for library header files

8 include all includes in the interface header (these should be in the implementation file to reduce the dependencies of the interface)

8 use <iostream> in header files (for interfaces that use the iostream, <iosfwd> should be used in the header files, it has enough details)

template <class BType, class IType = int>

4 make the template general

8 make the template as general as possible (I am unsure why there is an IType, it seems a little too general and unnecessary, why not just use an int? If Itype is used it will have knock on effect throughout the implementation and should not be mixed with int)

protected:

8 use protected for implementation members (these should be private to this class, in fact they could be removed from the interface by using a pimpl, pointer to an implementation class that hold them)

int lobound, hibound;

8 use implicit conversion (these variables are used to hold values of type IType and so should be declared as such, comparison and conversions with ITypes may/will cause problems!)

Array(int SZ = 16, IType lo = 0);

4 use sensible default values for when none are specified

8 using all uppercase letters for variables is fine (there are those who would only use all uppercase for preprocessor defines)

Array(const Array &x);

4 use const when passing a parameter as a reference

~Array();

4 explicit constructors paired with explicit destructor

IType lo() const; // return this->lobound

4 make member functions const if they do not alter the state of the object

8 comments that state the obvious are good (this is an implementation detail, either put the implementation inline here or hide it)

8 comments do not need to be accurate (comments if given must be kept up to date, this comment is wrong)

BType& operator[](IType i);

8 only need to define a non-const member function (you will not be able to read Array subscripts from a const object without casting)

Array<BType, IType>& operator= (const Array<Btype, IType> &x);

8 returned references need not be const (making the return type const would forces the declaration to be an rvlaue. Would this be safer here?)

8 need to restate template types (the <BType, IType> does not need to be restated for type Array. However you may state a new types for Array, that of the object to be copied, template<class BBType, classIIType> const Array& operator=( const Array<BBType, IIType>& x))

void grow(int);

8 no need to add parameter names (add them and make them helpful)

// implementation

8 it is OK to place implementation in headers (place it in its own .c. If this header is included multiple times the code will be compiled multiple times. Also note that as long as the interface in the header file stays constant, the implementation file may change with no need to compile other code. See earlier comments on #includes and pimpl classes)

template<class BType, class IType>

4 It is assumed that the implementation has been moved and therefore the template<class BType, class IType> will be required before each member definition

4 the default type, IType=int, should only be stated once, in the header

Array<BType, IType>::Array(int SZ, IType lo)

4 the default values, int SZ=16, Itype lo=0, should only be stated once, in the header

hibound(lo + SZ -1)

8 mixed type arithmetic of unknown types is fine (what if IType is a signed char? See comments on the use of IType above)

8 may use previously initialised values (it is OK here as lolound is declared before hibound but they could be swapped around, viz: int hibound, lobound; and then hibound would be initialised first)

assert(SZ > 0)

8 use assert to trap incorrect values (assert should be used as a debugging aid. This line will be removed in a release build when NDEBUG is defined and negative values will pass through the code unhindered. This comment stands for the other asserts too!)

arrayData = new BType[SZ];
assert(arrayData != 0);

8 always check the pointer is valid (and if it is not? Either new will throw an exception or return null in which case the caller must deal with it!)

(*this) = x; // use assignment

8 factor out similar code ( (*this) = x == (*this).operator=(x) == this->operator=(x) == operator=(x) but is this the way to do it? Is it safe to use this in a constructor?)

cout << "Array index " << i << endl;

4 use endl to force a '\n' and flush the stream

8 no need to state using namespace std (either state it or use std::cout and std::endl)

8 the called should report (this restricts the usefulness of the member function and reporting would be better handled, if desired, by the caller)

4 IType will handle the cout<< or we will get a compile time

errorassert (hibound-lobound 
      == x.hibound-x.lobound);

8 restrict usage (see comment above about assert, also the code could be made to handle different sized arrays)

arrayData[i] = const_cast<
    Array<BType, IType>&>(x).arrayData[i];

8 use const_cast to force read access of const objects (we should not need to use a const_cast, unless Btype does not have an operator[] const member, see comments above about operator[])

8 use interface to access implementation (in this case it seems excessive, as this is a private implementation we could directly access the data and use copy())

8 don't worry about roll back (this is not exception neutral)

There may be other subtle lessons I am being taught through this code that I am unaware of. As a novice I would welcome corrective comments. As an aside you may be interested to know that I initially learnt C from H.Schilt and may still have lessons that need unlearning.

These implementations try to follow the style of the code used already!

// idea 1
template <class BType, class IType>
void Array<BType, IType>::grow(int newS)
{
  assert(newS >= hibound-lobound);
// implicit conversion. 
// No checking done if NDEBUG is defined
  BType* newarrayData = new BType [newS];
  assert( newarrayData != 0); 
// no checking done if NDEBUG is defined
// similar code as operator=, hopefully
// hibound - lobound is less than INT_MAX
  for(int i = 0; i<= hibound - lobound; ++i)
    newarrayData[i] = (*this).arrayData[i];
  ~Array();        // delete old data
  arrayData=newarrayData; // insert new data
  hibound = lobound - newS + 1;// new length
  cout << "done" << endl;  
    // I just couldn't resist!
}

// idea 2
template <class BType, class IType>
void Array<BType, IType>::grow(int newS)
{
  assert(newS >= hibound-lobound);
  // implicit cast, don't add any further 
  // run time checking
  Array tempArray(newS,lobound);  
  // object of new length
  int swaphibound = tempArray.hibound; 
// keep record for later
  tempArray.hibound = hibound;  
// force to same size
  tempArray = (*this);    
// and copy data across using operator=
  BType *swaparrayData = arrayData;  
// swap the blocks over
  arrayData = tempArray.arrayData;
  tempArray.arrayData = swaparrayData;
  hibound = swaphibound;    
// and fix the extended size the old data will
// be destroyed when tempArray goes out of scope
}

N.B. In true student style, I have not tried to compile the two submissions. I think this is justifiable, the exercise would not have compiled either without alterations. (I did say the implementations would follow the style of the code!)

As I knew the code was poor I found myself carefully checking the syntax against the C++ Programming Language, which I found of great value. However in the context of the exercise I found myself practising the poor techniques that had been presented. The exercise could have been an opportunity to practise and become more familiar with a classic design pattern, something that I need to do. As it is I have only become familiar and worked with some very dubious methodologies.

After working through the whole exercise I still have no answers, except, bad habits. I will now go and purify my mind by reading what C++ Programming Language has to say about templates. I really must buy Design Patterns.