HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

C++ operator overloading for matrix operations

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
operationsoverloadingoperatorformatrix

Problem

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
#include
#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;icols=other.cols;
this->rows=other.rows;
this->Mat=new int* [other.rows];
assert(this->Mat);
for(int i =0;iMat[i]=new int[other.cols];
assert(this->Mat[i]);
}
for(int i=0;iMat[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;iMat[i][j]=otherM.Mat[i][j];
}
}

Matrix Matrix::operator-()const
{
Matrix temp(this->rows,this->cols)

Solution

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 . 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 


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

#include 


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 a

Code Snippets

Matrix  operator -()const;
Matrix  Matrix::operator -(const Matrix &t)const;
Matrix operator-(const Matrix &t) const;
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 ;
}
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;
}
#include <assert.h>

Context

StackExchange Code Review Q#149506, answer score: 3

Revisions (0)

No revisions yet.