C++ operator overloading for matrix operations

Question

Please check the code to see if there anything I can improve . Everyting is working as expected.

matrix.h

#ifndef Matrix_h
#define Matrix_h
using namespace std;
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 &otherM )const;
    Matrix  operator -()const;
    Matrix  Matrix::operator -(const Matrix &t)const;
    Matrix  Matrix::operator +(const Matrix &t)const ;
    Matrix  Matrix::operator *(const Matrix &t)const;
    Matrix  Matrix::operator *(const int &num)const;

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


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


    friend ostream & operator <<(ostream & os , const Matrix & m )
    {
        for(int i=0;i<m.rows;i++)
        {
          for(int j=0;j<m.cols;j++)
            cout <<m.Mat[i][j]<<"  " ;
           cout <<endl ;
        }
    return os ;
    }
};
#endif 

matrix.cpp

#include <iostream>
#include<assert.h>
#include "Matrix.h"
using namespace std;

Matrix::Matrix(const int &rows,const int &cols )//constructor of class Matrix
{
    this->rows=rows; 
    this->cols=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
{
    this->cols=other.cols;
    this->rows=other.rows;
    this->Mat=new int* [other.rows];
    assert(this->Mat);
    for(int i =0;i<other.rows;i++)
    {
       this->Mat[i]=new int[other.cols];
       assert(this->Mat[i]);
    }
    for(int i=0;i<other.rows;i++)
      for(int j=0;j<other.cols;j++)
            this->Mat[i][j]=other[i][j];
}


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

void Matrix::operator=(const Matrix &otherM )const
{
    if(this->Mat !=otherM.Mat && this->cols==otherM.cols && this->rows==otherM.rows)
     {
       for(int i=0;i<rows;i++)
        for(int j=0;j<cols;j++)
            this->Mat[i][j]=otherM.Mat[i][j];
     }
}


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


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


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

}

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

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

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

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

}

main

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

using namespace std;
void main()
{
    Matrix m(2, 2);
    m[0][0] = 2;
    m[1][1] = 2;
    cout << m << endl;
    m = m;
    const Matrix s = -m;
    cout << m << endl << s << endl;
    m = s+2 * -m * m * 2 - s;
    cout << m << endl << s << endl;
    cout << s[1][1] << endl;
}

Answer 1

I have found a number of things that could help you improve your code.

Fix your formatting

There are abundant examples here of C++ code that is well formatted. This code has peculiar and inconsistent indentation that makes it difficult to tell when a function begins and ends. Fixing that would improve readability.

Don't use std::endl unless you really need to flush the stream

The difference between std::endl and '\n' is that std::endl actually flushes the stream. This can be a costly operation in terms of processing time, so it's best to get in the habit of only using it when flushing the stream is actually required. It's not for this code.

Don't overspecify member functions

The code currently includes these two lines in Matrix.h:

Matrix  operator -()const;
Matrix  Matrix::operator -(const Matrix &t)const;

The first one is fine, but the second one is overspecified. Because it's already inside the Matrix class, it's not necessary to tell the compiler that it's inside the Matrix class. It makes the code more cluttered and harder to read as well as being inconsistent, so that second line shoud instead be written like this:

Matrix operator-(const Matrix &t) const;

Don't use void main()

The only function signatures allowed by the C++ standard both require an int return type. See this question for details.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It's particularly bad to use it when writing include headers.

Make sure you have all required #includes

The Matrix.h file refers to std::ostream uses atoi but doesn't #include <iostream>. Also, carefully consider which #includes are part of the interface (and belong in the .h file, as with this one) and which are solely part of the implementation (and therefore should only be listed in the .cpp file.)

Always put the class' own include first

If you get into the habit of always putting the #include file for the class first, you'll never have to worry about having insufficient include files in the header. That is, for this case, if the first line of Matrix.cpp had been #include "Matrix.h", your compiler would have been able to have pointed out the previous point to you.

Fix your inserter

There is a friend in the code that looks like this:

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++)
            cout <<m.Mat[i][j]<<"  " ;
        cout <<endl ;
    }
    return os ;
}

The error is that it is ignoring the pass std::ostream and using cout instead. I'd write it like this:

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;
}

Note that I've used the pre-increment rather than post-increment operators (not a big deal here, but a good habit to use) and introduced more whitespace to make it easier to read. I've also added curly braces to make it very clear to the person reading the code what was actually intended.

Use the C++ form of #include files

The code includes this line

#include <assert.h>

However that puts everything into the global namespace. Better would be to explicitly use the C++ version which looks like this:

#include <cassert>

This puts things into the std:: namespace and is a better way to do this for the standard C headers.

Don't use this-> in member functions

Using this->rows in a constructor is distracting and unhelpful. Simply use rows instead. If you're trying to avoid a name clash between passed parameters and internal variables, simply rename one of them. One common idiom is to prefix every member data item with m_.

Throw exceptions rather than asserts

It's good you're checking for problems in the code, but the C++ way of handling those kinds of things is by throwing an exception rather than termminating the program via an assert.

Allow for range checking

Because rows and cols are private member functions, and because operator[] does no range checking, there is no way for the user to provide range checking because there is no way to recover the dimensions of the matrix.

Make your base class destructor virtual

By default, a compiler-generated destructor is concrete rather than virtual, but this leads to problems with collections of objects. See this question for more details on why.

Consider return a reference from operator=

The current operator= has this signature:

void operator=(const Matrix &otherM ) const;

This has two problems that I can see. First, it shouldn't be const because it clearly alters the object (by definition!). Second, declaring it as return void isn't necessarily wrong, but it would prevent doing assignment chaining and returning the value of an assignment. Consider:

int a, b, c, d;
a = b = c = 5;
if (10 == (d = c+a)) {
    std::cout << "Addition works as expected\n";
}

That works just fine for int, but if we try that with your Matrix class it won't work for two reasons. First, there's no default constructor, and second, operator= returns void.

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.

Be wary of returning pointers to internal data

There is a serious problem with the implementation of operator[] because it returns, in effect, a pointer to internal data. Here are just two examples for why that's wrong:

int *kaboom;          // we use this later
const Matrix s = m;   // make a const copy
{ 
    const Matrix a = s;  // make a local copy
    a[1][1] = 5;         // this alters a const object! not good.
    kaboom = a[1];       // use operator[]
}  // a is now destroyed
kaboom[1] = 0;           // points to freed memory!  not good.

Answer 2

• It is very beneficial to define operator+() in terms of operator+=().

• operator +(const int &num,const Matrix &t) makes no mathematical sense. Besides, did you try to compile it?

• Adding (or subtracting) matrices of non-matching dimensions makes no sense, neither mathematical nor common. A condition

      if (this->rows!=t.rows ||this->cols!=t.cols)
  

  shall throw an exception, or signal an error in some appropriate way.

• this-> is superfluous. An unqualified name refers to a member anyways.