HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Simple array implementation without bounds-checking

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withoutsimplearraycheckingimplementationbounds

Problem

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 
    template 
    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 
      friend std::ostream& operator&);

    private:
      T* arr;
      int size;
    };

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

    template 
    Array::Array(const int& init_size, const T& init_value) {
      arr = new T[init_size];
      size = init_size;
      for (int i = 0; i 
    Array::Array(const Array& original) {
      size = original.size;
      arr = new T[size];
      for (int i = 0; i 
    Array::~Array() {
      delete [] arr;
    }

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

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

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

    template 
    Array& Array::operator= (const Array& copy) {
      if (arr == copy.arr)
        return *this;
     size = copy.size;
      delete [] arr;
      arr = new T[size];
      for (int i = 0; i 
    std::ostream& operator& self) {
      out << "[ ";
      for (int i = 0; i < self.size-1; ++i) {
        out << self.arr[i] << ", ";
      }
      out << self.arr[self.size-1] << " ]";
      return out;
    }

Solution

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

template 
Array::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 
Array::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 
Array& Array::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 
Array& Array::operator= (const Array& copy)
{
    if (this == ©)    // 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 
Array& Array::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& Array::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 
T Array::getVal(const int& index) const {
  return arr[index];
}

template 
void Array::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 
T& Array::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 
T const& Array::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.

Code Snippets

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;
}
template <class T>
Array<T>::Array(const int& init_size)
    : arr(new T[init_size]())
    , size(init_size)
{}
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;
}
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;
}
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.

Context

StackExchange Code Review Q#3944, answer score: 10

Revisions (0)

No revisions yet.