C++ operator overloading for matrix operations - follow-up

Question

This is improved code after I some issue in pointed by @Edward in the last question: C++ operator overloading for matrix operations

This work assignment in operator overloading .I need to use operators *, [][], =, +, -, << on objects of type matrix for example add to matrix using this code: m=m+s.

I already sent the code to my teacher but I still want your opinion so I can improve the next code.

matrix.h

#ifndef Matrix_h
#define Matrix_h
#include <iostream>


class Matrix
{
 private:
  int rows;
  int cols;
  int **Mat;


  public:
    Matrix (const int &rows,const int &cols);
    Matrix(const Matrix &other);
    ~Matrix ();
    int* & operator[](const int &index) const ;
    void operator=(const Matrix &other );
    Matrix  operator -()const;
    Matrix  operator -(const Matrix &other)const;
    Matrix  operator +(const Matrix &other)const ;
    Matrix  operator *(const Matrix &other)const;
    Matrix  operator *(const int &num)const;
    int getMatrixRows(const Matrix &other){return other.rows;}
    int getMatrixCols(const Matrix &other){return other.cols;}

    friend  Matrix operator *(const int & num,const Matrix &m)
    {
     return (m*num);
    }


    friend Matrix operator +(const int &num,const Matrix &t)
    {
     return (num+t);
    }




    friend std::ostream &operator<<(std::ostream &os, const Matrix &m) {
    for (int i=0; i < m.rows; ++i) {
        for (int j=0; j < m.cols; ++j) {
            os << m.Mat[i][j] << "  " ;
        }
        os << '\n';
    }
    return os;
}


};
#endif

matrix.cpp

#include "Matrix.h"
#include <iostream>
#include <cassert>


Matrix::Matrix(const int &n_rows,const int &n_cols )//constructor of class Matrix
{
    rows=n_rows; 
    cols=n_cols;
    Mat=new int* [cols];
    assert(Mat);
    for(int i =0;i<rows;i++)
    {
       Mat[i]=new int[cols];
       assert(Mat[i]);
    }
    for(int i=0;i<rows;i++)
      for(int j=0;j<cols;j++)
        Mat[i][j]=0;            
}




 Matrix::Matrix(const Matrix &other)  //copy constructor
{
    cols=other.cols;
    rows=other.rows;
    Mat=new int* [other.rows];
    assert(Mat);
    for(int i =0;i<other.rows;i++)
    {
       Mat[i]=new int[other.cols];
       assert(Mat[i]);
    }
    for(int i=0;i<other.rows;i++)
      for(int j=0;j<other.cols;j++)
            Mat[i][j]=other[i][j];
}





int* & Matrix::operator [](const int &index) const  // overloading operator []
{
  return  Mat [index];
}



void Matrix::operator=(const Matrix &other )   // overloading operator =
{
    if(Mat !=other.Mat && cols==other.cols && rows==other.rows)
     {
       for(int i=0;i<rows;i++)
        for(int j=0;j<cols;j++)
            Mat[i][j]=other.Mat[i][j];
     }
}





 Matrix  Matrix::operator-()const   // overloading operator -
{
    Matrix temp(rows,cols); 
    for(int i=0;i<rows;i++)
        for(int j=0;j<cols;j++)
            temp.Mat[i][j]=Mat[i][j]*-1;
   return temp;
}


 Matrix  Matrix::operator +(const Matrix &other)const  //add 2 matrix
{
    Matrix temp(rows,cols);  
    if (rows!=other.rows ||cols!=other.cols)
    {
       for(int i=0;i<rows;i++)
        for(int j=0;j<cols;j++)
            temp.Mat[i][j]=Mat[i][j];
       return temp;
    }
    else
     {      
         for(int i=0;i<rows;i++)
             for(int j=0;j<cols;j++)
                 temp.Mat[i][j]+=other.Mat[i][j]+Mat[i][j];
     }
    return temp; 
}





Matrix  Matrix::operator *(const Matrix &other)const   //multiplay matrix on the right
{
    if (cols!=other.rows)
    {
      Matrix temp(cols,rows);
      for(int i=0;i<rows;i++)
        for(int j=0;j<cols;j++)
            temp.Mat[i][j]=Mat[i][j];
      return temp;
    }
    else
    {
      Matrix temp(cols,other.rows);  
      for(int i=0;i<rows;i++)
          for(int j=0;j<other.cols;j++)
            for(int k =0;k<cols;k++)
                temp[i][j]+=Mat[i][k]*other.Mat[i][j];
      return temp;          
    }

}




Matrix  Matrix::operator *(const int &num)const   //multiplay with number
{
    Matrix temp(rows,cols);
    for(int i=0;i<rows;i++)
       for(int j=0;j<cols;j++)
            temp.Mat[i][j]=Mat[i][j]*num;
    return temp; 
}

Matrix  Matrix::operator -(const Matrix &other)const //matrix subtraction 
{
    Matrix temp(rows,cols);  
    if (rows!=other.rows ||cols!=other.cols)
    {
       for(int i=0;i<rows;i++)
        for(int j=0;j<cols;j++)
            temp.Mat[i][j]=Mat[i][j];
       return temp;
    }
    else
     {

         for(int i=0;i<rows;i++)
             for(int j=0;j<cols;j++)
                 temp.Mat[i][j]+=Mat[i][j]-other.Mat[i][j];
     }
    return temp;
}




Matrix::~Matrix ()//destrucor 
{
 for(int i =0;i<rows;i++)
   delete [] Mat[i]; 
 delete [] Mat;

}

main.cpp

#include "Matrix.h"
#include <iostream>

int main()
{
    Matrix m(2, 2);
    m[0][0] = 2;
    m[1][1] = 2;
    std::cout << m << std::endl;
    m = m;
    const Matrix s = -m;
    std::cout << m << std::endl << s << std::endl;
    m = s+2 * -m * m * 2 - s;
    std::cout << m << std::endl << s << std::endl;
    std::cout << s[1][1] << std::endl;

    return 0 ; 
}

I have been told to "throw exceptions rather than asserts" and to "make your base class destructor virtual". What is the right way to do it? I never used exception before and not familiar with the concept of virtual destructor.

Prefer a single allocation instead of doing multiple allocations in the constructor, it would be simpler to do only a single allocation. This is both faster and simpler

@Edward wrote this, but is it possible to allocate 2 dimensional array with an allocation?

Another thing I didn't understand is what to do when main is trying to use the function illegally for example add 2 matrix that not in the same size. I created a new object and gave him the same data as one then called the function and returned it. m=m+s in this example, if m and s are not in the same size I just returned new object with the values of m. Is it the right way?

Answer 1

Design

Looks OK. Though if you look at some more advanced versions of Matrix classes they have developed some good optimizations. Like multiplications (and other operations) are actually postponed until the resulting cells are actually required. This allows for optimizations where you can see that the result will be a specific value and you don't need to compute all the results for the final matrix).

C++ standard behaviors.

When you define operator* you usually also define operator*=. This allows for some good optimizations.

Example:

Matrix& operator*=(Matrix const& rhs) {
    /* Do Work */  // Here we may not need to allocate space
                   // for the result and it could potentially be done
                   // in place
    return *this;
}

The normal operation can then be easily defined in terms of this operator:

Matrix& operator*(Matrix const& rhs) {
    Matrix result(*this);
    return result *= rhs;
}

Now admittedly I have not thought this through completely for matrices (but this would be a normal pattern to follow).

Separation of Concerns

When you design classes you should try and separate resource management from business logic. Your code contains both (resource management is memory management and your business logic is all the matrix code).

Normally you should separate these out into two classes. Luckily you can do this automatically (because the resource management has already been done by std::vector<int>).

I would redefine the class to use std::vector to control the memory management part and thus just concentrate on the matrix operations in this class.

Code Review

Const Methods

Methods that do not change the state of the object should be marked const.

    int getMatrixRows(const Matrix &other){return other.rows;}
    int getMatrixCols(const Matrix &other){return other.cols;}

These only return a value. So they should be marked const so they can be used in a const context.

Bug

This looks like a recursive call that never returns.

    friend Matrix operator +(const int &num,const Matrix &t)
    {
     return (num+t);
    }

Did you write unit tests?

Initializing dynamic memory.

You don't need to manually reset all memory to zero:

for(int i=0;i<rows;i++)
  for(int j=0;j<cols;j++)
    Mat[i][j]=0;            

This can be done automatically during allocation by forcing zero construction of each integer vaule.

for(int i =0;i<rows;i++)
{
   Mat[i]=new int[cols]();
   //                  ^^  Notice this.
   //                      It forces zero initialization
   //                      rather than default initialization.
}

Assert and exception.

Your assert will never fire:

   assert(Mat[i]);

The call to new will never return nullptr, so the assert will never fire. If new fails, it will throw an exception.

One memory block rather than array of arrays.

You allocate an array of arrays. This means accessing an element becomes two memory accesses.

 matrix[4][3] // => Gets matrix[4] (a pointer)
              // => a pointer[3]   (second memory access)

If you allocate a single block of memory then you only have a single memory access. This does involve you calculating the location of the element:

  index = (4 * rowSize) + 3;
  return data(index);

A technique for overloading operator[] to do this can be found here How to overload array index operator for wrapper class of 2D array?.

Assignment Fail

Your assignment operator is broken.

You assume the destination array has enough space to be copied over. This is not generally the case. You will need to check.

The easiest method of implementing the assignment operator is called the Copy and Swap Idiom; you should look it up.

It means the assignment operator is implemented in terms of the copy constructor:

void Matrix::operator=(const Matrix &other)
{
    Matrix tmp(other);
    tmp.swap(*this);
    return *this;
}

Other quick notes

• Throw an exception: throw std::runtime_error("Error Message");
• Virtual Destructor: virtual ~Matrix() {/* Implement*/}
• Prefer Single allocation. You are allocating an array of pointers. Then for each pointer allocating an array of int. Rather than do this. Calculate the total area of the matrix N*M just do one allocation for all the elements. When somebody accesses [a][b], ou need to multiply a by the size of the row then add b to get the correct location of the element.
• If you try and multiply matrices of incompatible sizes, preferably this should be a compile time error (so that you can fix the code before it runs). If you don't know the size of your array until runtime then you should throw an exception to indicate that this is not allowed. Any errors should cause the code to stop working (so that you have to fix the error or explicitly do some coding to tell the user that something went wrong).

Answer 2

Since the swap is not a member function of Matrix. You will needs to implements the swap function in order to use

tmp.swap(*this);

a simple way is just the std::swap

std::swap(tmp,*this);

Also, the constructor should be initialized in this way (initialize a dynamic pointer to n-dimensional array)

Mat = new int*[rows]; // this part is missing, Mat is a dynamic array
for(int i = 0; i<rows; i++)
{
    Mat[i] = new int[cols]{0}; // initialize Mat[i] with cols number of 0.
}

https://github.com/AustinJia/matrix_multiplication_transpose