C++ Deep copy of dynamic array through assignment operator

Question

I am trying to copy a dynamically allocated array to an instance. My code seems to be copying the values over, but it also need to resize the array to match the "&other" size array.

A little info about the code: There are two classes at hand, one is "Movie" which takes a title, film-time, and director (all pointers) as private members. There is another called "MovieCollection" which is an array that stores each instance of "Movie" in a given index.

//These are private member variables:`

int ArrySize = 50; //There is another section of code that points to this and resizes if needed, I believe it needed a size at runtime though.

//Array to store instance of "movie"
Movie *movieArry = new Movie[ArrySize];

//This is assignment operator
const MovieCollection& operator=(const MovieCollection& other)
{ 
  delete []movieArray;
  int otherSizeArry = other.ArrySize;
  Movie* temp;
  temp = new Movie[otherSizeArry];

  for (int i = 0; i < otherSizeArry; i++)
  temp[i] = other.movieArry[i];

  return *this;
  delete []temp;
}

I used another function I wrote to resize the array while the instance is being created. For example, the instance I want to copy over has 10 indexes but the new instance I am trying to copy the values into still has a limit of 50. From what I understand I have to delete it because arrays cannot be resized, then copy the new size over (along with the values).

Any help would be greatly appreciated and thank you in advanced. Also, sorry if more code is required. I didn't want to give more than what was needed.

Answer 1

Your assignment operator is implemented incorrectly. It is freeing the movieArray array before allocating the new temp array. If the allocation fails, the class will be left in a bad state. And you are not assigning the temp array to movieArray before calling return *this; (the delete []temp is never reached, the compiler should have warned you about that).

The operator should look more like this instead:

MovieCollection& operator=(const MovieCollection& other)
{ 
    if (&other != this)
    {
        int otherSizeArry = other.ArrySize;
        Movie* temp = new Movie[otherSizeArry];

        for (int i = 0; i < otherSizeArry; ++i) {
            temp[i] = other.movieArry[i];
        }
        // alternatively:
        // std::copy(other.movieArry, other.movieArry + otherSizeArry, temp);

        std::swap(movieArray, temp);
        ArrySize = otherSizeArry;

        delete[] temp;
    }

    return *this;
}

If your class has a copy constructor (and it should - if it does not, you need to add one), the implementation of the assignment operator can be greatly simplified:

/*
MovieCollection(const MovieCollection& other)
{
    ArrySize = other.ArrySize;
    movieArray = new Movie[ArrySize];

    for (int i = 0; i < ArrySize; ++i) {
        movieArray[i] = other.movieArry[i];
    }
    // alternatively:
    // std::copy(other.movieArry, other.movieArry + ArrySize, movieArray);
}
*/

MovieCollection& operator=(const MovieCollection& other)
{ 
    if (&other != this)
    {
        MovieCollection temp(other);
        std::swap(movieArray, temp.movieArray);
        std::swap(ArrySize, temp.ArrySize);
    }

    return *this;
}