7
\$\begingroup\$

I am preparing myself for some job interviews. This is a simple array (with the error handling left out for brevity). Any input or suggestions on style and the use of templates would be appreciated.

    #include <iostream>
    template <class T>
    class Array {
    public:
      explicit Array(const int&);
      Array(const int&, const T&);
      Array(const Array&);
      virtual ~Array();
      T getVal(const int&) const;
      void setVal(const int&, const T&);
      T& operator[] (T);
      Array& operator= (const Array&);

      template <class U>
      friend std::ostream& operator<< (std::ostream&, const Array<U>&);

    private:
      T* arr;
      int size;
    };

    template <class T>
    Array<T>::Array(const int& init_size) {
      arr = new T[init_size];
      size = init_size;
    }

    template <class T>
    Array<T>::Array(const int& init_size, const T& init_value) {
      arr = new T[init_size];
      size = init_size;
      for (int i = 0; i < size; ++i) {
        arr[i] = init_value;
      }
    }

    template <class T>
    Array<T>::Array(const Array& original) {
      size = original.size;
      arr = new T[size];
      for (int i = 0; i < size; ++i) {
        arr[i] = original.arr[i];
      }
    }

    template <class T>
    Array<T>::~Array() {
      delete [] arr;
    }

    template <class T>
    T Array<T>::getVal(const int& index) const {
      return arr[index];
    }

    template <class T>
    void Array<T>::setVal(const int& index, const T& value) {
      arr[index] = value;
    }

    template <class T>
    T& Array<T>::operator[] (T index) {
      return arr[index];
    }

    template <class T>
    Array<T>& Array<T>::operator= (const Array& copy) {
      if (arr == copy.arr)
        return *this;
     size = copy.size;
      delete [] arr;
      arr = new T[size];
      for (int i = 0; i < size; ++i) {
        arr[i] = copy.arr[i];
      }
      return *this;
    }

    template <class T>
    std::ostream& operator<< (std::ostream& out, const Array<T>& self) {
      out << "[ ";
      for (int i = 0; i < self.size-1; ++i) {
        out << self.arr[i] << ", ";
      }
      out << self.arr[self.size-1] << " ]";
      return out;
    }
\$\endgroup\$
  • \$\begingroup\$ White space is your friend when it comes to readability. \$\endgroup\$ – Martin York Aug 7 '11 at 19:53
  • \$\begingroup\$ Oddly enough, I killed off much of the white space in order to post it. I know not to do that now. \$\endgroup\$ – Charles Aug 7 '11 at 22:41
10
\$\begingroup\$

In the constructor that just takes a size. I would initialize all members to their default value even if POD.

template <class T>
Array<T>::Array(const int& init_size)
{
  arr = new T[init_size]();
                  //    ^^^^^  Add braces. on types it makes no difference.
                  //           On POD it will force zero initialization.
  size = init_size;
}

Use the initializer lists in your constructors.

template <class T>
Array<T>::Array(const int& init_size)
    : arr(new T[init_size]())
    , size(init_size)
{}

Use copy and swap idiom for assignment operator (it automatically provides the strong exception guarantee).

template <class T>
Array<T>& Array<T>::operator= (const Array& copy)
{
    if (arr == copy.arr)  // This is the wrong test.
        return *this;     // Though it works as a side effect you will confuse the maintainer.

    size = copy.size;
    delete [] arr;       // breaks the strong exception gurantee.
                         // What it the next line fails? Then you are left with an object
                         // in an inconsistent state. (arr points at de-allocated memory)
    arr = new T[size];

    for (int i = 0; i < size; ++i)
    {
        arr[i] = copy.arr[i];
    }
    return *this;
}

If you want to do this the hard way then it should look like this:

template <class T>
Array<T>& Array<T>::operator= (const Array& copy)
{
    if (this == &copy)    // Return quickly on assignment to self.
    {   return *this;
    }

    // Do all operations that can generate an expception first.
    // BUT DO NOT MODIFY THE OBJECT at this stage.
    T* tmp = new T[size];
    for (int i = 0; i < size; ++i)
    {
        tmp[i] = copy.arr[i];
    }

    // Now that you have finished all the dangerous work.
    // Do the operations that  change the object.
    std::swap(tmp, arr);
    size = copy.size;

    // Finally tidy up
    delete tmp;    // Notice the swap above.

    // Now you can return
    return *this;
}

Alternatively use the copy and swap idiom

template <class T>
Array<T>& Array<T>::operator=(Array const& rhs)
{
    // 1 Copy
    Array    copy(rhs);     // Use the copy constructor to make a safe copy.

    // 2 Swap
    swap(arr,   copy.arr);
    swap(size,  copy.size);

} // 3 as the local copy goes out of scope it destroys the old array.

Note this can be optimized too:

Array<T>& Array<T>::operator=(Array rhs) // 1 implicit copy as parameter is passed by value
{
    // 2 Swap
    swap(arr,   copy.arr);
    swap(size,  copy.size);

} // 3 as the local copy goes out of scope it destroys the old array.

You don't need a get/set methods. Infact get/set is an obvious indication of bad design.

template <class T>
T Array<T>::getVal(const int& index) const {
  return arr[index];
}

template <class T>
void Array<T>::setVal(const int& index, const T& value) {
  arr[index] = value;
}

Just remove both these methods. What you want to do is return a reference to the internal object in your overload of operator[] (which you do) bit the index is an integer usually.

template <class T>
T& Array<T>::operator[] (size_t index)
{                //      ^^^^^^^    Changed type here.
  return arr[index];
}

You also need to provide a verision to use when your array is const.

template <class T>
T const& Array<T>::operator[] const (size_t index)
//^^^^^   Here                ^^^^^  and here.
{
  return arr[index];
}

If you provide an output operator you should generally by symmetric and provide an input operator.

\$\endgroup\$
  • \$\begingroup\$ Your review has proven to be extremely helpful. I am taking it all in and will attempt another data structure with these comments in mind. Thanks. \$\endgroup\$ – Charles Aug 7 '11 at 22:42
  • \$\begingroup\$ "Copy and swap" idiom is clever. \$\endgroup\$ – Charles Aug 7 '11 at 23:29
  • \$\begingroup\$ @Charles: I did not mention it. But if you look at the stl, all the containers provide an explicit swap() method. This allows assignment operator to be implemented as a single call to swap. This is done becuase you generally want to be able to implement the swap function (not method) as a simple call to a swap method anyway. Thus by having a swap method you keep the code in a single place. Note: swap() and destructor() are usually the two functions are declared as no throw operations. \$\endgroup\$ – Martin York Aug 8 '11 at 0:00
3
\$\begingroup\$

@Martin (in particular) has already given some excellent advice on a number of points, but I think there are a few more that are worth mentioning.

First of all, you've used the thrice-accursed array new expression to allocate your memory. Although this works (for a sufficiently loose definition of "works") it's not ideal, at least in my opinion.

I'd prefer to use ::operator new to allocate "raw" memory, and then using placement new to copy objects into the array.

Second, you've provided both an explicit constructor to create uninitialized storage, and an two-parameter constructor to create an Array initialized to a specified value. I think I'd prefer to condense the two into a single constructor with a default argument for the initial value, so you can never (even accidentally) create an Array of uninitialized data.

Third, if (for whatever reason) the user's copy ctor throws an exception while we're initializing our data, we want to ensure that we destroy (only) the objects we've created. Incorporating those, we end up with a ctor something like this:

template <class T>
Array<T>::Array(size_t init_size, const T& init_value) : 
    arr((T *)::operator new(init_size * sizeof(T))),
{
    for (size = 0; size<init_size; ++size)
        new(arr+size) T(init_value);
}

Note that instead of a local variable to walk through the array and initialize the values, we use size, so if an exception got thrown during the loop, size will contain a count of the items we successfully initialized. Of course, we need to modify the copy ctor to match:

template <class T>
Array<T>::Array(const Array &original) :
    arr((T *)::operator new(original.size * sizeof(T))),
{
    for (size = 0; size < original.size; ++size) 
        new(arr+size) T(original.arr[size]);
}

Then in our dtor, we can make use of that to destroy exactly/only the objects that were successfully constructed. To fit normal conventions, we destroy those in reverse order of creation:

template <class T>
Array<T>::~Array() {
    for (size_t i=size+1; i>0; --i)
        arr[i-1].~T();
    ::operator delete(arr);
}

To go with this, you (of course) need to modify your class definition to match:

template <class T>
class Array {
    T* arr;
    size_t size;
public:
    explicit Array(size_t, const T& init_value = T());
    Array(const Array&);
    ~Array();
    T& operator[] (size_t);
    Array& operator= (Array);
};

One last minor detail: you had your dtor marked as virtual, but since the class contains no other virtual functions, it doesn't seem to be particularly suited to use as a base class. That being the case, I've made the dtor non-virtual.

The other thing I think I'd do if I were writing a class like this would be to add enough for it to act as a container, as that's defined by the standard (define an iterator type, value_type, begin() and end() that return iterators, etc.)

\$\endgroup\$
1
\$\begingroup\$

Seeing as you provide operator[], you do not need to make operator<< a friend: you can be fairly certain that operator[] will be inlined, anyway (although you should provide a const version of it, as someone suggested).

Personally, I would use a scoped pointer for arr in order to make it more exception-safe (with NDEBUG enabled, it should not cause any runtime overhead). (The copy-and-swap idiom works for this, too.)

By the way, I'm guessing your operator[] taking a T was a typo -- it should take size_t, as should pretty much everything that currently takes int.

I don't see an issue with your getVal and setVal functions -- they do exactly the same thing as operator[], and are just a more verbose way of writing things out (more normal is an at function that does out-of-bounds checking and returns a reference, but not a big deal).

I wouldn't call your class Array, as this, in my mind, associates with things of a compile-time size -- at least DynArray would be more accurate.

Finally, a function that would be trivial to implement is size, and I think the lack of it could be very troublesome.

\$\endgroup\$
0
\$\begingroup\$

Any reason why you pass integers as const references - it will merely slow down your code, and possible confuse the reader, for no benefit.

I'm rusty on templates, so I could be wrong, but doesn't

  Array(const Array&);

Allow you to try to construct an Array from an Array - shouldn't it be

  Array(const Array<T>&);

Likewise for assignment operator.

Just a couple of thoughts off the top of my head on a Sunday evening.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.