Jump to content
  • Advertisement
Sign in to follow this  
sheep19

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.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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.

Share this post


Link to post
Share on other sites
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?

Share this post


Link to post
Share on other sites
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..

Share this post


Link to post
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.

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


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

Share this post


Link to post
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 this post


Link to post
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;


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.

Share this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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;

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!