C++ OOP - Arrays (matrices) multiplication, deleting object

Question

I am writing simple C++ application to multiply two matrices. Yes, I am aware this version doesn't check e.g. whether number of rows is greater than zero.

I can create two objects using a default constructor and I can create third object (a result of multiplication of two previous objects of the same class). I can't however correctly delete this third object. When I try, an "Access violation reading location" exception is throw. Can you tell me what am I missing? Is there something I forget to initialize? This exception is only thrown when using myThirdFunction.

Any other advice on making this code better is welcome :)

#include <iostream>
#include <ctime>
#include <iomanip>
using namespace std;

class MyArray
{
int Variable1 = 0;
int Variable2 = 0;
float ** myArray;
public:
MyArray() : Variable1(0), Variable2(0){ myFunction(); }

MyArray (int variable1, int variable2) : Variable1(variable1):
 Variable2(variable2){ myFunction(); }

MyArray(const MyArray &myArrayA, const MyArray &myArrayB) :
 Variable1(myArrayA.Variable1), Variable2(myArrayB.Variable2) 
{ myThirdFunction(myArrayA, myArrayB); }

MyArray(const MyArray &myArray) 
{ Variable1=myArray.Variable1; Variable2 = myArray.Variable2; 
this->myArray =myArray.myArray; }

void myFunction()   {
    cin >> Variable1;
    cin >> Variable2;
    myArray = new float*[Variable1];
    myOtherFunction();
}

void myOtherFunction()  {
    for (int i = 0; i < Variable1; ++i)
    {
        myArray[i] = new float[Variable2];
        for (int j = 0; j < Variable2; ++j)
            myArray[i][j] = rand() % (10 - 0) + 0;
    }       
}

void myThirdFunction(MyArray myArrayA, MyArray myArrayB)
{
    Variable1 = myArrayA.Variable1;
    Variable2 = myArrayB.Variable2;
    myArray = new float*[Variable1];
    for (int i = 0; i < Variable1; ++i)
    {
        myArray[i] = new float[Variable2];
        for (int j = 0; j < Variable2; ++j)
        {
            float tempVariable = 0;
            for (int q = 0; q < myArrayA.Variable2; ++q)
            {
                tempVariable += myArrayA.myArray[i][q] * myArrayB.myArray[q][j];
            }
            myArray[i][j] = tempVariable;
        }
    }
}

void displayMyArray()   {
    for (int i = 0; i < Variable1; ++i)
    {
        for (int j = 0; j < Variable2; ++j)
            cout << myArray[i][j] << '\t';
        cout << endl;
    }
}

~MyArray()  {
    for (int i = 0; i < Variable1; ++i)
    {
        delete[] myArray[i];
    }
    delete[]myArray;
}
};

int main(){
srand(time(0));
MyArray myArrayA;
myArrayA.displayMyArray();
cout << endl;
MyArray myArrayB;
myArrayB.displayMyArray();
cout << endl;
MyArray myArrayC(myArrayA, myArrayB);

myArrayC.displayMyArray();

getchar();
getchar();
return 0;
}

Thanks :)

Answer 1

The member myArray is a pointer, and in your copy constructor you just copy the pointer without creating a new array. Because myThirdFunction takes both arguments by value, it creates two copies of the original objects. And because the copy constructor doesn't create a new value, when those two copies go out of scope at the end of the function, the original pointers now point to deallocated memory. Whenever the original two objects are destroyed, the destructor tries to delete memory that was already deleted.

From the beginning, you have this constructor:

MyArray(const MyArray &myArrayA, const MyArray &myArrayB)
{
    myThirdFunction(myArrayA, myArrayB); //This call creates two new objects
}

The signature of myThirdFunction being void myThirdFunction(MyArray myArrayA, MyArray myArrayB), you pass by value and create two new copies. That calls the copy constructors for the two parameters:

MyArray(const MyArray &myArray) 
{
    this->myArray =myArray.myArray; //shallow copy, very bad
}

The new objects now point to the same memory as the originals. So, when they are destroyed at the end of myThirdFunction, the original pointers become garbage. In a broad view, this happens. float* p = new float; delete p; delete p; The solution is to make the copy constructor actually copy the elements, not the pointers:

MyArray(const MyArray &p_copy) //The name confused me so I changed it
{
    Variable1 = p_copy.Variable1;
    Variable2 = p_copy.Variable2; 
    myArray new float*[Variable1];
    for (int i = 0; i < Variable1; ++i)
    {
        myArray[i] = new float[Variable2];
        for (int j = 0; j < Variable2; ++j)
            myArray[i][j] = p_copy.myArray[i][j];
    }
}

In addition, you probably want to pass to myThirdFunction by constant reference.

void myThirdFunction(const MyArray& myArrayA, const MyArray& myArrayB)

I don't see a need to create two temporary objects here.

Answer 2

At the first time this code will works.

#include <iostream>
#include <ctime>
#include <iomanip>
using namespace std;

class MyArray
{
    int Variable1;
    int Variable2;
    float ** myArray;
public:
    MyArray() : myArray(NULL), Variable1(0), Variable2(0) // initialize pointers with NULL is important 
    { 
        myFunction(); 
    }

    MyArray (int variable1, int variable2) : myArray(NULL), Variable1(variable1),
     Variable2(variable2)
    { 
        myFunction(); 
    }

    MyArray(const MyArray &myArrayA, const MyArray &myArrayB) : myArray(NULL),
     Variable1(myArrayA.Variable1), Variable2(myArrayB.Variable2) 
    { 
        myThirdFunction(myArrayA, myArrayB); 
    }

    MyArray(const MyArray &myArray) 
    { 
        Variable1=myArray.Variable1; Variable2 = myArray.Variable2; 
        this->myArray =myArray.myArray; 
    }

    void myFunction()   
    {
        cin >> Variable1;
        cin >> Variable2;
        myArray = new float*[Variable1];
        myOtherFunction();
    }

    void myOtherFunction()  
    {
        for (int i = 0; i < Variable1; ++i)
        {
            myArray[i] = new float[Variable2];
            for (int j = 0; j < Variable2; ++j)
                myArray[i][j] = rand() % (10 - 0) + 0;
        }       
    }

    void myThirdFunction(MyArray myArrayA, MyArray myArrayB)
    {
             // cols of first must be same as rows of second
        if (myArrayA.Variable2 != myArrayB.Variable1)
            return;

            // memory must be cleaned before new array creation
        clearArray();

        Variable1 = myArrayA.Variable1;
        Variable2 = myArrayB.Variable2;

        myArray = new float*[Variable1];
        for (int i = 0; i < Variable1; ++i)
        {
            myArray[i] = new float[Variable2];
            for (int j = 0; j < Variable2; ++j)
            {
                float tempVariable = 0;
                for (int q = 0; q < myArrayA.Variable2; ++q)
                {
                    tempVariable += myArrayA.myArray[i][q] * myArrayB.myArray[q][j];
                }
                myArray[i][j] = tempVariable;
            }
        }
    }

    void displayMyArray()   
    {
        for (int i = 0; i < Variable1; ++i)
        {
            for (int j = 0; j < Variable2; ++j)
                cout << myArray[i][j] << '\t';
            cout << endl;
        }
    }

    ~MyArray()  
    {
        clearArray();
    }

   // clear memory and deinitialize pointers
    void clearArray()
    {
        if (myArray == NULL)
            return;

        for (int i = 0; i < Variable1; ++i)
        {
            delete[] myArray[i];
            myArray[i] = NULL;
        }

        delete[]myArray;
        myArray = NULL;
    }

};

int main(){
    srand(time(0));
    MyArray myArrayA;
    myArrayA.displayMyArray();
    cout << endl;
    MyArray myArrayB;
    myArrayB.displayMyArray();
    cout << endl;
    MyArray myArrayC(myArrayA, myArrayB);

    myArrayC.displayMyArray();

    getchar();
    getchar();
    return 0;
}

but I strongly recomend you to create proper copy constructor and assignment operator means (operator=) to proper object creation and destruction.

Here I post my own matrix realization to refer you in what way you can approve your code.

#include <iostream.h>
#include <conio.h>
#include <stdlib.h>

using namespace std::

class Matrix
{
public:
   Matrix();
   Matrix(int rowcount,int colcount);
   Matrix(int rowcount,int colcount,float* matrix);
   Matrix(const Matrix& rhs);
   ~Matrix();
   /////////////////////////////////////////////////////////////
   Matrix& operator = (const Matrix& rhs);
   Matrix operator + (const Matrix& rhs);
   Matrix operator - (const Matrix& rhs);
   Matrix operator * (float scale);
   Matrix operator * (const Matrix& rhs);
   void   operator += (const Matrix& rhs);
   void   operator -= (const Matrix& rhs);
   void   operator *= (float scale);
   void   operator *= (const Matrix& rhs);
   float  operator [] (int offset) const;
   float& operator [] (int offset);
   friend ostream& operator << (ostream& _str,const Matrix& rhs);
   /////////////////////////////////////////////////////////////
   void setCols(int cols);
   void setRows(int rows);
   void setMatrix(float* matrix);
   int getCols() const
   {
    return itsCols;
   }
   int getRows() const
   {
    return itsRows;
   }
   const float* getMatrix() const
   {
    Invariants();
    return itsMatrix;
   }
   void Invariants() const
   {
       if ((!(itsCols && itsRows && itsMatrix)) && (itsRows < 0) && (itsCols < 0))
       {
      cout << "Not allowed action!\n";
      getch();
      exit(0);
       }
   }
   /////////////////////////////////////////////////////////////
private:
   float* itsMatrix;
   int itsRows;
   int itsCols;
};


Matrix::Matrix()
{
   itsRows = 0;
   itsCols = 0;
   itsMatrix = NULL;
}

Matrix::Matrix(int rowcount,int colcount)
{
   itsRows = rowcount;
   itsCols = colcount;
   itsMatrix = new float[itsRows * itsCols];
   Invariants();
}

Matrix::Matrix(int rowcount,int colcount,float* matrix)
{
   itsRows = rowcount;
   itsCols = colcount;
   itsMatrix = new float[itsCols * itsRows];
   int counter = 0;
   for (int i = 0; i < itsRows; i++)
   {
    for (int j = 0; j < itsCols; j++)
    {
        itsMatrix[counter] = matrix[counter];
        counter++;
    }
   }
   Invariants();
}

Matrix::Matrix(const Matrix& rhs)
{
   itsCols = rhs.getCols();
   itsRows = rhs.getRows();
   itsMatrix = new float[itsRows * itsCols];
   int counter = 0;
   for (int i = 0; i < itsRows; i++)
   {
      for (int j = 0; j < itsCols; j++)
      {
      itsMatrix[counter] = rhs[counter];
      counter++;
      }
   }
}

Matrix::~Matrix()
{
   itsCols = 0;
   itsRows = 0;
   delete [] itsMatrix;
   itsMatrix = NULL;
}

Matrix& Matrix::operator = (const Matrix& rhs)
{
   if (&rhs == this)
      return *this;
   else
   {
      itsRows = rhs.getRows();
      itsCols = rhs.getCols();
      delete [] itsMatrix;
      itsMatrix = NULL;
      itsMatrix = new float[itsRows * itsCols];
      int counter = 0;
      for (int i = 0; i < itsRows; i++)
      {
      for (int j = 0; j < itsCols; j++)
      {
          itsMatrix[counter] = rhs[counter];
          counter++;
      }
      }
      return *this;
   }
}

float& Matrix::operator [] (int offset)
{
   Invariants();
   if ((offset > -1) && (offset < itsCols * itsRows))
      return itsMatrix[offset];
   else
   {
      cout << "You cann't reach this element!\n";
      getch();
      exit(0);
   }
   return itsMatrix[offset];
}

float Matrix::operator [] (int offset) const
{
   Invariants();
   if ((offset > -1) && (offset < itsCols * itsRows))
       return itsMatrix[offset];
   else
   {
       cout << "You cann't reach this element!\n";
       getch();
       exit(0);
   }
   return 0;
}

Matrix Matrix::operator + (const Matrix& rhs)
{
    Invariants();
    if (!((this->itsCols == rhs.getCols()) &&
    (this->itsRows == rhs.getRows())))
    {
       cout << "Cann't perform addiction of matrixes!\n";
       getch();
       exit(0);
    }
    Matrix temp(itsRows,itsCols);
    int counter = 0;
    for (int i = 0; i < itsRows; i++)
    {
       for (int j = 0; j < itsCols; j++)
       {
      temp[counter] = itsMatrix[counter] + rhs[counter];
      counter++;
       }
    }
    return temp;
}

Matrix Matrix::operator - (const Matrix& rhs)
{
    Invariants();
    if (!((this->itsCols == rhs.getCols()) &&
    (this->itsRows == rhs.getRows())))
    {
       cout << "Cann't perform substraction of matrixes!\n";
       getch();
       exit(0);
    }
    Matrix temp(itsRows,itsCols);
    int counter = 0;
    for (int i = 0; i < itsRows; i++)
    {
    for (int j = 0; j < itsCols; j++)
    {
        temp[counter] = itsMatrix[counter] - rhs[counter];
        counter++;
    }
    }
    return temp;
}

Matrix Matrix::operator * (float scale)
{
    Invariants();
    Matrix temp(itsRows,itsCols);
    int counter = 0;
    for (int i = 0; i < itsRows; i++)
    {
    for (int j = 0; j < itsCols; j++)
    {
         temp[counter] = itsMatrix[counter] * scale;
         counter++;
    }
    }
    return temp;
}

Matrix Matrix::operator * (const Matrix& rhs)
{
    Invariants();
    if (!(itsCols == rhs.getRows()))
    {
    cout << "Cann't perform multiplication of matrixes!\n";
    getch();
    exit(0);
    }
    Matrix temp(itsRows,rhs.getCols());
    int counter = 0;
    float sum = 0;
    for (int i = 0; i < itsRows; i++)
    {
    for (int j = 0; j < rhs.getCols(); j++)
    {
         for (int k = 0; k < itsCols; k++)
         {
         sum += itsMatrix[i * itsCols + k] *
         rhs[k * rhs.getCols() + j];
         }
         temp[counter] = sum;
         sum = 0;
         counter++;
    }
    }
    return temp;
}

void Matrix::operator += (const Matrix& rhs)
{
    if (!((this->itsCols == rhs.getCols()) &&
    (this->itsRows == rhs.getRows())))
    {
       cout << "Cann't perform addiction of matrixes!\n";
       getch();
       exit(0);
    }
    Matrix temp(itsRows,itsCols);
    int counter = 0;
    for (int i = 0; i < itsRows; i++)
    {
       for (int j = 0; j < itsCols; j++)
       {
      temp[counter] = itsMatrix[counter] + rhs[counter];
      counter++;
       }
    }
    *this = temp;
}

void Matrix::operator -= (const Matrix& rhs)
{
    if (!((this->itsCols == rhs.getCols()) &&
    (this->itsRows == rhs.getRows())))
    {
       cout << "Cann't perform substraction of matrixes!\n";
       getch();
       exit(0);
    }
    Matrix temp(itsRows,itsCols);
    int counter = 0;
    for (int i = 0; i < itsRows; i++)
    {
    for (int j = 0; j < itsCols; j++)
    {
        temp[counter] = itsMatrix[counter] - rhs[counter];
        counter++;
    }
    }
    *this = temp;
}

void Matrix::operator *= (float scale)
{
    Invariants();
    Matrix temp(itsRows,itsCols);
    int counter = 0;
    for (int i = 0; i < itsRows; i++)
    {
    for (int j = 0; j < itsCols; j++)
    {
         temp[counter] = itsMatrix[counter] * scale;
         counter++;
    }
    }
    *this = temp;
}

void Matrix::operator *= (const Matrix& rhs)
{
    Invariants();
    if (!(itsCols == rhs.getRows()))
    {
    cout << "Cann't perform multiplication of matrixes!\n";
    getch();
    exit(0);
    }
    Matrix temp(itsRows,rhs.getCols());
    int counter = 0;
    float sum = 0;
    for (int i = 0; i < itsRows; i++)
    {
    for (int j = 0; j < rhs.getCols(); j++)
    {
         for (int k = 0; k < itsCols; k++)
         {
         sum += itsMatrix[i * itsCols + k] *
         rhs[k * rhs.getCols() + j];
         }
         temp[counter] = sum;
         sum = 0;
         counter++;
    }
    }
    *this = temp;
}

ostream& operator << (ostream& _str,const Matrix& rhs)
{
    rhs.Invariants();
    int counter = 0;
    for (int i = 0; i < rhs.getRows(); i++)
    {
    for (int j = 0; j < rhs.getCols(); j++)
    {
        _str << rhs[counter] << "\t";
        counter++;
    }
    _str << endl;
    }
    return _str;
}

float arr1[] =
{
    2, 2, 2,
    -1, -3, -5,
    16, 8, 24,
    8,  0, 16
};

float arr2[] =
{
    15,
    2,
    -4
};

int main()
{
   clrscr();
   Matrix m1(4,3,arr1);
   Matrix m2(3,1,arr2);
   cout << "Matrix 1:\n";
   cout << m1 << endl;
   cout << "Matrix 2:\n";
   cout << m2 << endl;
   cout << "Matrix 1 * Matrix 2\n";
   cout << m1 * m2 << endl;
   getch();
   cout << "Matrix 1 + Matrix 1\n";
   cout << m1 + m1 << endl;
   getch();
   cout << "Matrix 1 - Matrix 1\n";
   cout << m1 - m1 << endl;
   getch();
   cout << "Matrix 1 * 4\n";
   cout << m1 * 4 << endl;
   getch();
   return 0;
}