patterncppMinor
C++ operator overloading for matrix operations
Viewed 0 times
operationsoverloadingoperatorformatrix
Problem
Please check the code to see if there anything I can improve .
Everyting is working as expected.
matrix.h
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)
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 ;
}
};
#endifmatrix.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
Don't overspecify member functions
The code currently includes these two lines in
The first one is fine, but the second one is overspecified. Because it's already inside the
Don't use
The only function signatures allowed by the C++ standard both require an
Don't abuse
Putting
Make sure you have all required
The
Always put the class' own include first
If you get into the habit of always putting the
Fix your inserter
There is a
The error is that it is ignoring the pass
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
The code includes this line
However that puts everything into the global namespace. Better would be to explicitly use the C++ version which looks like this:
This puts things into the
Don't use
Using
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
Allow for range checking
Because
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
The current
This has two problems that I can see. First, it shouldn't be
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 stdPutting
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
#includesThe
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 filesThe 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 functionsUsing
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 aCode 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.