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

• Mast: You say it's working as expected, but you don't tell us what it's supposed to do. You've also been told not to use using namespace std in your code at your previous question, yet you're still using it. Please elaborate both points.
• Mast: On top of that, you're probably using multiple files. The formatting has made it one big code block. Try adding the file names in between for better context and splitting the code blocks as a side effect.
• benz: sorry I need to use operator overloading on matrix object. Things like * - + print using << and access specific cell with [] [] Regarding the issue with using namespace std I simply forgot :) I need to change it –
• Edward: I have rolled back the last edit. Please see what you may and may not do after receiving answers .

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.

• benz: first of all thank you for the very detailed answer ! I change the format of matrix.cpp file is it better ? I removed the std::endl from my files in main file it's code I got from my teacher so I can't change it . You told me to "Throw exceptions rather than asserts" what is the right way to do it ()? "Make your base class destructor virtual" I really don't understand what that means ? "Consider return a reference from operator=" I remvoed the const and I understand the concept but there is no need to return refernce in this assingmnet The new code in first comment
• benz: can you please check the things I changed in the code ?
• Edward: Changing the question after you've received answers invalidates the answers and isn't allowed. The way to do it is to do everything you can to improve the code and then post a new question.

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.

• Toby Speight: It's telling that there's no test for that operator+(int, Matrix) given that it appears to be defined in terms of itself! Perhaps it has been compiled, but I'm sure it hasn't been exercised.