Avoid repeating the same code in copy constructor and operator=

Question

In c++ when classes contains dynamically allocated data it is usually reasonable to explicitly define copy constructor, operator= and destructor. But the activity of these special methods overlaps. More specifically operator= usually first does some destruction and then it does coping similar to the one in copy constructor.

My question is how to write this the best way without repeating the same lines of code and without the need for processor to do unnecessary work (like unnecessary copying).

I usually end up with two helping methods. One for construction and one for destruction. The first is called from both copy constructor and operator=. The second is used by destructor and operator=.

Here is the example code:

    template <class T>
    class MyClass
    {
        private:
        // Data members
        int count;
        T* data; // Some of them are dynamicly allocated
        void construct(const MyClass& myClass)
        {
            // Code which does deep copy
            this->count = myClass.count;
            data = new T[count];
            try
            {
                for (int i = 0; i < count; i++)
                    data[i] = myClass.data[i];
            }
            catch (...)
            {
                delete[] data;
                throw;
            }
        }
        void destruct()
        {
            // Dealocate all dynamicly allocated data members
            delete[] data;
        }
        public: MyClass(int count) : count(count)
        {
            data = new T[count];
        }
        MyClass(const MyClass& myClass)
        {
            construct(myClass);
        }
        MyClass& operator = (const MyClass& myClass)
        {
            if (this != &myClass)
            {
                destruct();
                construct(myClass);
            }
            return *this;
        }
        ~MyClass()
        {
            destruct();
        }
    };

Is this even correct? And is it a good habit to split the code this way?

Answer 1

One initial comment: the operator= does not start by destructing, but by constructing. Otherwise, it will leave the object in an invalid state if the construction terminates via an exception. Your code is incorrect because of this. (Note that the necessity to test for self assignment is usually a sign that the assignment operator is not correct.)

The classical solution for handling this is the swap idiom: you add a member function swap:

void MyClass:swap( MyClass& other )
{
    std::swap( count, other.count );
    std::swap( data, other.data );
}

which is guaranteed not to throw. (Here, it just swaps an int and a pointer, neither of which can throw.) Then you implement the assignment operator as:

MyClass& MyClass<T>::operator=( MyClass const& other )
{
    MyClass tmp( other );
    swap( tmp );
    return *this;
}

This is simple and straight forward, but any solution in which all operations which may fail are finished before you start changing the data is acceptable. For a simple case like your code, for example:

MyClass& MyClass<T>::operator=( MyClass const& other )
{
    T* newData = cloneData( other.data, other.count );
    delete data;
    count = other.count;
    data = newData;
    return *this;
}

(where cloneData is a member function which does most of what your construct does, but returns the pointer, and doesn't modify anything in this).

EDIT:

Not directly related to your initial question, but generally, in such cases, you do not want to do a new T[count] in cloneData (or construct, or whatever). This constructs all of the T with the default constructor, and then assigns them. The idiomatic way of doing this is something like:

T*
MyClass<T>::cloneData( T const* other, int count )
{
    //  ATTENTION! the type is a lie, at least for the moment!
    T* results = static_cast<T*>( operator new( count * sizeof(T) ) );
    int i = 0;
    try {
        while ( i != count ) {
            new (results + i) T( other[i] );
            ++ i;
        }
    } catch (...) {
        while ( i != 0 ) {
            -- i;
            results[i].~T();
        }
        throw;
    }
    return results;
}

Most often, this will be done using a separate (private) manager class:

//  Inside MyClass, private:
struct Data
{
    T* data;
    int count;
    Data( int count )
        : data( static_cast<T*>( operator new( count * sizeof(T) ) )
        , count( 0 )
    {
    }
    ~Data()
    {
        while ( count != 0 ) {
            -- count;
            (data + count)->~T();
        }
    }
    void swap( Data& other )
    {
        std::swap( data, other.data );
        std::swap( count, other.count );
    }
};
Data data;

//  Copy constructor
MyClass( MyClass const& other )
    : data( other.data.count )
{
    while ( data.count != other.data.count ) {
        new (data.data + data.count) T( other.date[data.count] );
        ++ data.count;
    }
}

(and of course, the swap idiom for assignment). This allows multiple count/data pairs without any risk of loosing exception safety.

Answer 2

Implement the assignment by first copying the right-hand side and then swapping with that. This way you also get exception safety, which your code above doesn't provide. You could end up with a broken container when construct() fails after destruct() succeeded otherwise, because the member pointer references some deallocated data, and on destruction that will be deallocated again, causing undefined behaviour.

foo&
foo::operator=(foo const& rhs)
{
   using std::swap;
   foo tmp(rhs);
   swap(*this, tmp);
   return *this;
}

Answer 3

I don't see any inherent problem with that, as long as you make sure not to declare construct or destruct virtual.

You might be interested in chapter 2 in Effective C++ (Scott Meyers), which is completely devoted to constructors, copy operators and destructors.

As for exceptions, which your code is not handling as it should, consider items 10 & 11 in More effective C++ (Scott Meyers).