Return to Revisions

Thanks for reading my article. :-)

###Three Syntax Errors:

template <typename T>
ArrayList<T>& ArrayList<T>::operator =(const ArrayList<T>& other)
 // ^^^ Missing
template <typename T>
ArrayList<T>& ArrayList<T>::operator =(ArrayList<T>&& other)
 // ^^^ Missing
 
template <typename T>
ArrayList<T>::ArrayList(const ArrayList<T>& other)
{
 arr = static_cast<T*>(::operator new(sizeof(T)*size));
 // ^^^^ not defined in scope
 (did you mean actualSize)

###Stop doing this

using namespace std;

Doing this is a header file will pollute the namespace for all other applications that include your header file. This will piss other people off to no end (it can also break other people's code).

Doing it in the source file (for anything larger than a 10 line program is dangerous). The potential for name conflicts are high and just because there is a conflict does not mean there will be an error (as the compiler has leeway to do auto conversion).

See Why is "using namespace std" in C++ considered bad practice?

###Default constructor You don't need a default and a version that takes an integer.

 ArrayList();
 ArrayList(int size);

This can be achieved by a single constructor that has a defaulted size parameter.

 ArrayList(int size = 100);

###Move Semantics

You should mark your move constructor/assignment as none throwing (unless they can actually throw)

 ArrayList(ArrayList&& other) noexcept;
 ArrayList<T>& operator= (ArrayList&& other) noexcept;
 // ^^^^^^^^

Doing so allows for better compiler optimizations. The standard library is designed in a way that certain optimizations are possible if the move operations are non throwing. As this allows the strong exception guarantee on their containers when objects are moved (thus allowing move over copy).

###Swap as a member

I write swap as a member function. So I can mark it as noexcept. This then allows you to call it from your noexcept move constructor/assignment. Your friend version of swap can then call the member function.

 friend void swap(ArrayList& A, ArrayList& B)
 {
 using std::swap;
 swap(A.actualSize, B.actualSize);
 swap(A.allocatedSize, B.allocatedSize);
 swap(A.arr, B.arr);
 }

I would have done:

 void swap(ArrayList& rhs) noexcept
 {
 using std::swap;
 swap(actualSize, rhs.actualSize);
 swap(allocatedSize, rhs.allocatedSize);
 swap(arr, rhs.arr);
 } 
 friend void swap(ArrayList& lhs, ArrayList& rhs)
 {
 lhs.swap(rhs);
 }
 

###Throwing

 if(size < 0)
 throw;

This is valid syntactically. But semantically it is only allowed when there is a propagating exception that has been caught (at which point it re-throws the current exception).

You need to throw an explicit exception object here.

 if(size < 0)
 {
 throw std::runtime_error("Size out of range");
 }

###Prefer to use initializer lists

template <typename T>
ArrayList<T>::ArrayList()
{
 arr = static_cast<T*>(::operator new(sizeof(T)*100));
 actualSize = 0;
 allocatedSize = 100;
}

In this case it makes no difference. BUT it can so it is a good habit to get into. Prefer to use initializer lists to prevent objects being initialized twice during construction.

template <typename T>
ArrayList<T>::ArrayList(int size = 100)
 : ar(allocMemory(size))
 , actualSize(0)
 , allocatedSize(size);
{}

###Copying can throw.

template <typename T>
ArrayList<T>::ArrayList(const ArrayList<T>& other)
{
 arr = static_cast<T*>(::operator new(sizeof(T)*size));
 allocatedSize = other.allocatedSize;
 // If you set `actual Size` here.
 // Then call `add()` will this not increase the actual
 // size even more? (you also probably ment actualSize = other.actualSize;)
 actualSize = actualSize;
 // If any of these copies throws.
 // Then you leak all previous copies and
 // and the array `arr`. 
 for(size_t i = 0; i<other.actualSize; i++)
 add(other.arr[i]);
 // Note the destructor of the object is only called if
 // if the constructor completes.
}

You need to add some more checking here to make this safe:

template <typename T>
ArrayList<T>::ArrayList(const ArrayList<T>& other)
 : ArrayList(other.allocatedSize)
{
 try
 {
 for(size_t i = 0; i<other.actualSize; ++i)
 {
 add(other.arr[i]);
 }
 }
 catch(...)
 {
 for(size_t i = actualSize; i > 0; --i)
 {
 arr[i-1].~T();
 }
 ::operator delete(arr);
 throw;
 }
}

###Move Constructor

You don't initialize the members in the constructor. Since POD members don't be default get zero initialized (as an automatic object) they will have random values in them.

template <typename T>
ArrayList<T>::ArrayList(ArrayList&& other)
{
 // So you are swapping the random values of this
 // object into other.
 swap(*this, other);
}

You should do this:

template <typename T>
ArrayList<T>::ArrayList(ArrayList&& other)
 : arr(nullptr)
 , actualSize(0)
 , allocatedSize(0);
{
 // So you are swapping the random values of this
 // object into other.
 swap(*this, other);
}

###Move Assignment

Needs a return.

template <typename T>
ArrayList& ArrayList<T>::operator =(ArrayList<T>&& other)
{
 swap(*this, other);
 return *this; // Add this line.
}

###Destructor

When you create objects in an array. When the array is destroyed the objects are destructed in reverse order of creation. You should probably mimic that in your destructor.

template <typename T>
ArrayList<T>::~ArrayList()
{
 for(size_t i = 0; i<actualSize; i++)
 {
 arr[i].~T(); // I would use arr[actualSize - i - 1].~T();
 }::operator delete(arr);
// ^^^^ You probably want a new line
}

###Resize size:

The value of 2 sounds good. But 1.5 is better.

http://lokiastari.com/blog/2016/03/25/resizemaths/

 // This looks exactly like a copy constructor.
 // So why not create a private copy constructor that
 // allows an increased pre-allocated size.
 ArrayList tmp(allocatedSize);
 for(size_t i=0; i<actualSize; i++)
 addInternal(arr[i]);

Something like this:

private:
template <typename T>
ArrayList<T>::ArrayList(const ArrayList<T>& other, int allocatedSize)
 : ArrayList(allocatedSize)
{
 try
 {
 for(size_t i = 0; i<other.actualSize; ++i)
 {
 add(other.arr[i]);
 }
 }
 catch(...)
 {
 for(size_t i = actualSize; i > 0; --i)
 {
 arr[i-1].~T();
 }
 ::operator delete(arr);
 throw;
 }
}
public:
// public copy constructor uses the private version
// just passing a couple of parameters forward.
template <typename T>
ArrayList<T>::ArrayList(const ArrayList<T>& other)
 ArrayList(other, other.allocatedSize)
{}
template <typename T>
void ArrayList<T>::resize()
{
 // resize() uses the private copy constructor
 // but passes an increased size through.
 ArrayList tmp(*this, std::max(allocatedSize * 1.5, 2));
 swap(*this, tmp);
}

Remove is incorrect.

template <typename T>
void ArrayList<T>::remove(int index)
{
 if(index<0 || index>actualSize - 1)
 throw; // need an exception object
 for(size_t i=0; i<actualSize; i++)
 {
 if(index==i)
 {
 // Once the object is destroyed you can not use it.
 // You can not copy into or out of this object as
 // it no longer exists.
 arr[i].~T();
 // You are moving the elements in the wrong direction.
 // And you just copied from a destroyed object.
 arr[i+1] = arr[i];
 }
 else if(index > i)
 arr[i+1] = arr[i];
 }
}

What you want to do is move the object all down a position. Then destroy the one that is at the end of the array.

template <typename T>
void ArrayList<T>::remove(int index)
{
 if(index<0 || index >= actualSize)
 {
 throw std::runtime_error("Invalid Range");
 }
 for(i = index + 1; i < actualSize; ++i)
 {
 arr[i-1] = std::move(arr[i]);
 }
 
 // Once you have moved all the items down one place
 // Then destroy the last item.
 arr[actualSize-1].~T();
 --actualSize;
}

default