My Matrix class. How well is it implemented?

Started by
36 comments, last by DevFred 15 years, 4 months ago
I've made a Matrix class. Please post your thoughts about how to improve it.

#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

	//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);

	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)
{
	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)
			{
				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.
Advertisement
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?
SlimDX | Ventspace Blog | Twitter | Diverse teams make better games. I am currently hiring capable C++ engine developers in Baltimore, MD.
Quote:Original post by Promit
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?


How can I do that? The return type is Matrix..
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.

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

I suggest
#ifdef _DEBUGif(!can_be_added(*this, m)) {/*error handling*/}#endif...
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]
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;


5) Delete your show() method. That's what your std::ostream << operator overload is for.

6) Make it possible to retrieve a single element from within the matrix.
[size=2][ I was ninja'd 71 times before I stopped counting a long time ago ] [ f.k.a. MikeTacular ] [ My Blog ] [ SWFer: Gaplessly looped MP3s in your Flash games ]
#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.
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.
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];}
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;

This topic is closed to new replies.

Advertisement