# My Matrix class. How well is it implemented?

This topic is 3661 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

#ifndef MATRIX_H
#define MATRIX_H

#define NULL 0

#include <iostream>

using std::cout;

const char ERROR_MESSAGE[][80] =
{
"Assignment error. The dimensions do not match.\n",
"Addition error. The dimensions do not match.\n",
"Subtraction error. The dimensions do not match.\n",
"Multiplication error. The dimensions do not match.\n"
};

enum ERROR_NUM {ASSIGNMENT, ADDITION, SUBTRACTION, MULTIPLICATION};

class Matrix
{
public:
Matrix(int rows, int columns);	// constructor
Matrix(const Matrix &m); // copy constructor
~Matrix();	// destructor

Matrix& operator =(const Matrix &m); // assignment operator
Matrix operator +(const Matrix &m); // increment operator
Matrix operator -(const Matrix &m);
Matrix operator *(const Matrix &m);

friend std::ostream& operator <<(std::ostream &os, const Matrix &m);
void show() const;

int get_rows() const {return rows;}
int get_columns() const {return columns;}

static inline bool can_be_added(const Matrix &a, const Matrix &b)
{
return (a.rows == b.rows && a.columns == b.columns);
}
static inline bool can_be_subtracted(const Matrix &a, const Matrix &b)
{
return (a.rows == b.rows && a.columns == b.columns);
}
static inline bool can_be_multiplied(const Matrix &a, const Matrix &b)
{
return (a.columns == b.rows);
}

private:
int **ppMatrix, rows, columns;
};

#endif // MATRIX_H


#include "Matrix.h"

Matrix::Matrix(int rows, int columns): rows(rows), columns(columns)
{
ppMatrix = new int*[rows];
for(int i = 0; i < rows; ++i)        // Allocate memory for the matrix
ppMatrix = new int[columns];

for(int i = 0; i < rows; ++i)		// Set values
for(int j = 0; j < columns; ++j)
ppMatrix[j] = rand() % 10;
}

Matrix::Matrix(const Matrix &m) // copy constructor
{
// the copy constructor creates an independant object
rows = m.rows;
columns = m.columns;
ppMatrix = new int*[rows];

for(int i = 0; i < rows; ++i)
ppMatrix = new int[columns];

for(int i = 0; i < rows; ++i)		// Set values
for(int j = 0; j < columns; ++j)
ppMatrix[j] = m.ppMatrix[j];
}

Matrix::~Matrix() // destructor
{
// for every row, delete columns
for(int i = 0; i < rows; ++i)
delete []ppMatrix;
}

Matrix& Matrix::operator =(const Matrix &m)
{
if(rows == m.rows && columns == m.columns) // if the matrices can be assigned
{
if(this != &m) // if we aren't assigning to itself
{
//the assignment operator is the destructor + the copy constructor

//first, delete the currect matrix and free the memory
for(int i = 0; i < rows; ++i)
delete []ppMatrix;

//then, create the new matrix
rows = m.rows;
columns = m.columns;
ppMatrix = new int*[rows];

for(int i = 0; i < rows; ++i)
ppMatrix = new int[columns];

for(int	i = 0; i < rows; ++i)		// set values
for(int j = 0; j < columns; ++j)
ppMatrix[j] = m.ppMatrix[j];
}
return *this;
}
else
cout << ERROR_MESSAGE[ASSIGNMENT];
}

Matrix Matrix::operator +(const Matrix &m)
{
{
Matrix temp(rows, columns);

for(int i = 0; i < rows; ++i)
for(int j = 0; j < columns; ++j)
temp.ppMatrix[j] = ppMatrix[j] + m.ppMatrix[j];

return temp;
}
else
}

Matrix Matrix::operator -(const Matrix &m)
{
if(can_be_subtracted(*this, m) )
{
Matrix temp(rows, columns);

for(int i = 0; i < rows; ++i)
for(int j = 0; j < columns; ++j)
temp.ppMatrix[j] = ppMatrix[j] - m.ppMatrix[j];

return temp;
}
else
cout << ERROR_MESSAGE[SUBTRACTION];
}
Matrix Matrix::operator *(const Matrix &m)
{
if(can_be_multiplied(*this, m) )
{
Matrix temp(this->rows, m.columns);

for(int i = 0; i < temp.rows; ++i)
{
for(int j = 0; j < temp.columns; ++j)
{
int s = 0;
for(int k = 0; k < this->columns; ++k) // this->columns = m.rows
{
s += ppMatrix[k] * m.ppMatrix[k][j];
}
temp.ppMatrix[j] = s;
}
}

return temp;
}
else
cout << ERROR_MESSAGE[MULTIPLICATION];
}

void Matrix::show() const
{
for(int i = 0; i < rows; ++i)
{
for(int j = 0; j < columns; ++j)
{
cout << ppMatrix[j] << " ";
}
cout << "\n";
}
cout << "\n";
}

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

return os;
}


Edit: I will overload +, - and * to work with integers as well.

##### Share on other sites
Your error handling system is cout? Really? How, exactly, am I supposed to catch that? And why would I want separate error codes depending on the operation involved? That doesn't help at all. Have you considered using assert instead?

Also, why doesn't show() just use the operator you define right after it?

Next, your operators don't actually return anything if they detect an error. The compiler would have flagged this. Did you not run this through a compiler, or did you just ignore what it said?

##### Share on other sites
Quote:
 Original post by Promityour operators don't actually return anything if they detect an error. The compiler would have flagged this. Did you not run this through a compiler, or did you just ignore what it said?

How can I do that? The return type is Matrix..

##### Share on other sites
1) Why are the matrix elements ints? Why don't you use floats (or a template)?

2) Why does the constructor creates a random matrix? You could add different creator functions like identity(m,n), zero(m,n), random(m,n)

3) Is there any way to access individual elements?

4) The assignment operator should check for self-assignment.

if(can_be_added(*this, m))    {       ....    }    else        cout << ERROR_MESSAGE[ADDITION];

I suggest
#ifdef _DEBUGif(!can_be_added(*this, m)) {/*error handling*/}#endif...

##### Share on other sites
Your destructor and operator= leak memory as they don't delete ppMatrix after having deleted all it's contents.

Also there is no need to free and reallocate memory in operator= because you have already checked that the current array has the correct size.

If performance is an issue, I think allocating all the needed space (both the space used by elements and pointers to rows) with a single call to new would be a good choise, because using many memory allocations is slow and increases memory fragmentation.

[Edited by - Sisu on November 27, 2008 12:35:50 PM]

##### Share on other sites
1) Never use any using directives in a header file (i.e. don't say using std::cout).

2) Use assert like Promit suggested. I prefer assert over Kambiz's proposed method because it's shorter, easier, and standard.

3) Make your Matrix class templated so it can store anything and not just ints.

4) You're leaking memory all over the place. Make sure that for everything you new, you also delete it before reassigning it (or letting it get destroyed). Basically you should delete like this:

for (int r = 0; r < rows; r++){    delete [] ppMatrix[r];}delete [] ppMatrix;

6) Make it possible to retrieve a single element from within the matrix.

##### Share on other sites
#ifndef MATRIX_H#define MATRIX_H#define NULL 0#include <iostream>#include <sstream>#include <string>using std::cout;	using std::cin;const char ERROR_MESSAGE[][80] = {	"Assignment error. The dimensions do not match.\n",	"Addition error. The dimensions do not match.\n",	"Subtraction error. The dimensions do not match.\n",	"Multiplication error. The dimensions do not match.\n"};enum ERROR_NUM {ASSIGNMENT, ADDITION, SUBTRACTION, MULTIPLICATION};float get_float();class Matrix{public:	Matrix(int rows, int columns);	// constructor	Matrix(const Matrix &m); // copy constructor	~Matrix();	// destructor	//overloaded operators	Matrix& operator =(const Matrix &m); // assignment operator	Matrix operator +(const Matrix &m); // increment operator	Matrix operator -(const Matrix &m);	Matrix operator *(const Matrix &m);	float operator()(unsigned i, unsigned j);	friend std::ostream& operator <<(std::ostream &os, const Matrix &m);	friend std::istream& operator >>(std::istream &os, Matrix &m);		void show() const;	void set_to_0(); // set all elements to 0	void set_random(int range_start, int range_finish); //set random values	void set_values();	int get_rows() const {return rows;}	int get_columns() const {return columns;}	static inline bool can_be_added(const Matrix &a, const Matrix &b)	{		return (a.rows == b.rows && a.columns == b.columns);	}	static inline bool can_be_subtracted(const Matrix &a, const Matrix &b)	{		return (a.rows == b.rows && a.columns == b.columns);	}	static inline bool can_be_multiplied(const Matrix &a, const Matrix &b)	{		return (a.columns == b.rows);	}private:	int rows, columns;	float **ppMatrix;};#endif // MATRIX_H

#include "Matrix.h"Matrix::Matrix(int rows, int columns): rows(rows), columns(columns){	ppMatrix = new float*[rows];	for(int i = 0; i < rows; ++i)        // Allocate memory for the matrix		ppMatrix = new float[columns];	set_to_0();}Matrix::Matrix(const Matrix &m) // copy constructor{	// the copy constructor creates an independant object	rows = m.rows;	columns = m.columns;	ppMatrix = new float*[rows];	for(int i = 0; i < rows; ++i)		ppMatrix = new float[columns];	for(int i = 0; i < rows; ++i)		// Set values		for(int j = 0; j < columns; ++j)			ppMatrix[j] = m.ppMatrix[j];}Matrix::~Matrix() // destructor{	// for every row, delete columns	for(int i = 0; i < rows; ++i)		delete []ppMatrix;	delete ppMatrix;}Matrix& Matrix::operator =(const Matrix &m){	if(rows == m.rows && columns == m.columns) // if the matrices can be assigned	{		if(this != &m) // if we aren't assigning to itself		{			//the assignment operator is the destructor + the copy constructor			//first, delete the currect matrix and free the memory			for(int i = 0; i < rows; ++i)				delete []ppMatrix;			delete ppMatrix;			//then, create the new matrix			rows = m.rows;			columns = m.columns;			ppMatrix = new float*[rows];			for(int i = 0; i < rows; ++i)				ppMatrix = new float[columns];			for(int	i = 0; i < rows; ++i)		// set values				for(int j = 0; j < columns; ++j)					ppMatrix[j] = m.ppMatrix[j];		}		return *this;	}	else		cout << ERROR_MESSAGE[ASSIGNMENT];}Matrix Matrix::operator +(const Matrix &m){	if(can_be_added(*this, m) )	{		Matrix temp(rows, columns);		for(int i = 0; i < rows; ++i)			for(int j = 0; j < columns; ++j)				temp.ppMatrix[j] = ppMatrix[j] + m.ppMatrix[j];		return temp;	}	else		cout << ERROR_MESSAGE[ADDITION];}Matrix Matrix::operator -(const Matrix &m){	if(can_be_subtracted(*this, m) )	{		Matrix temp(rows, columns);		for(int i = 0; i < rows; ++i)			for(int j = 0; j < columns; ++j)				temp.ppMatrix[j] = ppMatrix[j] - m.ppMatrix[j];		return temp;	}	else		cout << ERROR_MESSAGE[SUBTRACTION];}Matrix Matrix::operator *(const Matrix &m){	if(can_be_multiplied(*this, m) )	{		Matrix temp(this->rows, m.columns);		for(int i = 0; i < temp.rows; ++i)		{			for(int j = 0; j < temp.columns; ++j)			{				float s = 0.0f;				for(int k = 0; k < this->columns; ++k) // this->columns = m.rows				{					s += ppMatrix[k] * m.ppMatrix[k][j];				}				temp.ppMatrix[j] = s;			}		}			return temp;	}	else		cout << ERROR_MESSAGE[MULTIPLICATION];}void Matrix::show() const{	cout << *this;}std::ostream& operator <<(std::ostream &os, const Matrix &m){	for(int i = 0; i < m.rows; ++i)	{		for(int j = 0; j < m.columns; ++j)		{			os << m.ppMatrix[j] << " ";		}		os << "\n";	}	os << "\n";	return os;}std::istream& operator >>(std::istream &os, Matrix &m){	for(int i = 0; i < m.rows; ++i)	{		for(int j = 0; j < m.columns; ++j)		{			cout << "Enter [" << i << "][" << j << "] of " << "[" << m.rows << "][" << m.columns << "]: ";			m.ppMatrix[j] = get_float();		}	}	return os;}void Matrix::set_to_0(){	for(int i = 0; i < rows; ++i)		for(int j = 0; j < columns; ++j)			ppMatrix[j] = 0;}void Matrix::set_random(int range_start, int range_finish){	for(int i = 0; i < rows; ++i)		for(int j = 0; j < columns; ++j)			ppMatrix[j] = range_start + rand() % (range_finish - range_start);}void Matrix::set_values(){	cin >> *this;}float get_float(){	std::string temp;	std::getline(std::cin, temp);	std::stringstream ss(temp);	float f;	ss >> f;	return f;}float Matrix::operator ()(unsigned i, unsigned j){	if(i < rows && j < columns)		return ppMatrix[j];}

Is there a way to make operator() not return a garbage value? Should I use a pointer and return null?

Now I need to use that assert.

##### Share on other sites
If one of the row allocations in your constructor fails then your destructor will then happily try to delete memory that your application does not own. This is really all down to exception safety and proper use of RAII.

I would separate out the storage policy for your matrix class. Most of the time I would expect that statically sized (3x3 and 4x4) matrices are suitable; your implementation only offers dynamically sized ones.

If I were making a more serious implementation then there would be a richer set of operators and matrix expressions would be evaluated at compile-time using expression templates.

##### Share on other sites
Quote:
 Original post by sheep19 *** Source Snippet Removed ****** Source Snippet Removed ***Is there a way to make operator() not return a garbage value? Should I use a pointer and return null?Now I need to use that assert.

If i and j is out of boundary you won't return anything from operator(), and that is really bad. You compiler should warn you about that. If it isn't you need to either turn up the warning levels, or get a better compiler.

You should probably put the test code in an assert instead of the if, so that the program aborts if your out of bounds:
float Matrix::operator ()(unsigned i, unsigned j){	assert(i < rows && j < columns && "Matrix indexes out of bounds!");	return ppMatrix[j];}

##### Share on other sites
these 3 should be const....
	Matrix operator +(const Matrix &m) const; // increment operator	Matrix operator -(const Matrix &m) const;	Matrix operator *(const Matrix &m) const;

1. 1
2. 2
3. 3
Rutin
23
4. 4
5. 5
khawk
14

• 9
• 11
• 11
• 23
• 12
• ### Forum Statistics

• Total Topics
633653
• Total Posts
3013166
×