Copy constructor of dynamic two-dimensional array

Question

I need help. Now i'm trying to make a class Matrix, but my program freezes every time I run it in Visual Studio 2013. I think there is some trouble with copy constructor. Here is the whole code. `

class Matrix
{
private:
    int** matrix;
    int X; // Matrix rows
    int Y; // Matrix columns
public:
    // Default Constructor
    Matrix()
    {
        X = 0;
        Y = 0;
        matrix = NULL;
    }
    // Constructor with parameters
    Matrix(int _X, int _Y)
    {
        srand(time(NULL));

        X = _X;
        Y = _Y;
        matrix = new int*[X];

        for (int i = 0; i < X; i++)
                matrix[i] = new int[Y];

        for (int i = 0; i < X; i++)
        {
            for (int j = 0; j < Y; j++)
            {
                matrix[i][j] = rand() % 100;
            }
        }

    }
    // Copy constructor
    Matrix(const Matrix& N)
    {
        X = N.X;
        Y = N.Y;

        matrix = new int*[X];

        for (int i = 0; i < X; i++)
            matrix[i] = new int[Y];

        for (int i = 0; i < X; i++)
        {
            for (int j = 0; j < Y; j++)
            {
                matrix[i][j] = N.matrix[i][j];
            }
        }
    }
    // Destructor
    ~Matrix()
    {
        for (int i = 0; i < X; i++)
            delete[] matrix[X];
    }
    //--------------------------------------
    void ShowMatrixOnScreen()
    {
        for (int i = 0; i < X; i++)
        {
            for (int j = 0; j < Y; j++)
                cout << matrix[i][j] << "   ";

            cout << endl << endl;
        }
    }
};

void main() 
{
    Matrix x(4, 2);
    x.ShowMatrixOnScreen();
}

`

Function "ShowMatrixOnScreen" prints matrix "x" on a screen, but then console freezes.

Answer 1

Your destructor has the statement delete[] matrix[X];

matrix[X] does not exist, so you are freeing memory which is not allocated to you, which is Undefined Behavior. It should be delete [] matrix[i] instead!

Moreover, as pointed by Vlad From Moscow, it is advisable to delete all the memory, so you shall also consider adding delete [] matrix, to delete the 1-D array of pointers which you allocated using matrix = new int*[X];

Answer 2

The problem is that your destructor is invalid

First of all in this loop

    for (int i = 0; i < X; i++)
        delete[] matrix[X];

it tries to access non-existent element matrix[X] beyond the allocated array. The valid range for indices is [0, X - 1]

So you have to write

    for (int i = 0; i < X; i++)
        delete[] matrix[i];
                       ^^^

But thsi loop is valid only if matrix is not equal to nullptr Therefore before the loop you have to check whether matrix is equal to nullptr or not equal.

And also you need to delete the memory allocated to the first one-dimensionl array

delete [] matrix;

Thus the destructor will look like

~Matrix()
{
    if ( matrix )
    {
        for ( int i = 0; i < X; i++ ) delete []matrix[i];
    }

    delete []matrix;
}

Also in the copy constructor you should also check whether data member matrix of the copied object also is not equal to nullptr

// Copy constructor
Matrix(const Matrix& N)
{
    X = N.X;
    Y = N.Y;

    if ( N.matrix )
    {
        matrix = new int*[X];

        for (int i = 0; i < X; i++)
            matrix[i] = new int[Y];

        for (int i = 0; i < X; i++)
        {
            for (int j = 0; j < Y; j++)
            {
                matrix[i][j] = N.matrix[i][j];
            }
        }
    }
}

It would be much better if data members X and Y has type size_t. Otherwise you need also to check whether they are not negative.

The allocation of the memory could be placed in a separate private member function.

for example

int ** allocate( size_t m, size_t n )
{
    int **p = nullptr;

    if ( n != 0 && m != 0 )
    {
        matrix = new int *[m];

        for ( size_t i = 0; i < m; i++ ) matrix[i] = new int[n];
    }

    return p;
}

In the class definition data members X and Y should precede data member matrix

private:
    int X; // Matrix rows
    int Y; // Matrix columns    
    int** matrix;

In this case you could use mem-initializer list of constructors to initialize the class data members.

For example

// Constructor with parameters
Matrix(int X, int Y) : X( X ), Y( Y ), matrix( allocate( X, Y ) )
{
   if ( matrix )
   {
       srand(time(NULL));

        for (int i = 0; i < X; i++)
        {
            for (int j = 0; j < Y; j++)
            {
                matrix[i][j] = rand() % 100;
            }
        }
    }
}

And the default constructor could be defined like

// Default Constructor
Matrix() : Matrix( 0, 0 )
{
}

In turn the copy constructor could be defined like

// Copy constructor
Matrix( const Matrix &N ) : X( N.X ), Y( N.Y ), matrix( allocate( N.X, N.Y ) )
{
    if ( matrix )
    {
        for (int i = 0; i < X; i++)
        {
            for (int j = 0; j < Y; j++)
            {
                matrix[i][j] = N.matrix[i][j];
            }
        }
    }
}

And you should define also the copy assignment operator or define it as deleted. Otherwise there will be problems when you try to assign one object of the class to another.

Take into account that function main shall have return type int

int main()