C++ matrix class

Question

I wrote a matrix class in C++ with an addition and multiplication method. It works fine, but I don't think a C++ programmer would enjoy reading this. I would love some help with that.

#include <iostream>
#include <string>
class Matrix {
public:
 int rows;
 int columns;
 int* contents;
 // Constructor
 // data is the contents of the matrix in a list
 // like you read a book
 Matrix(int m, int n, int* data) {
 rows = m;
 columns = n;
 
 // We copy data to a dynamically allocated piece of memory
 // to make the program more robust.
 contents = new int[m*n];
 
 for (int i = 0; i < m*n; i++) {
 contents[i] = data[i]; 
 }
 } 
 
 // Returns the element in row i, column j
 int getElement(int i, int j) {
 return contents[columns*i + j];
 }
 // Gets the i'th row of this matrix
 int* getRow(int i) {
 // Dynamic because we need the row outside this helper function
 int* row = new int[columns];
 
 for (int j = 0; j < columns; j++) {
 row[j] = getElement(i, j);
 }
 
 return row;
 }
 
 // Gets a the j'th column of matrix other
 int* getColumn(int j) {
 // Dynamic because we need the column outside this helper function
 int* column = new int[rows];
 
 for (int i = 0; i < rows; i++) {
 column[i] = getElement(i, j);
 }
 
 return column;
 }
 
 Matrix addition(Matrix other) {
 int* sum = new int[rows*columns];
 
 for (int i = 0; i < rows*columns; i++) {
 sum[i] = contents[i] + other.contents[i]; 
 }
 
 Matrix sumMatrix(rows, columns, sum);
 
 delete sum;
 
 return sumMatrix;
 }
 
 Matrix product(Matrix other) {
 int* product = new int[rows*other.columns];
 
 for (int i = 0; i < rows; i++) {
 for (int j = 0; j < other.columns; j++) {
 int* row = getRow(i);
 int* column = other.getColumn(j);
 int value = innerProduct(row, column, columns); // Our row and the other guy's column contain as many elements as we have columns
 
 delete row;
 delete column;
 product[other.columns*i + j] = value;
 }
 }
 
 Matrix productMatrix(rows, other.columns, product);
 
 delete product;
 
 return productMatrix;
 }
 
 void print() {
 
 for (int i = 0; i < rows; i++) {
 std::cout << '{';
 for (int j = 0; j < columns; j++) {
 std::cout << getElement(i, j) << ", ";
 }
 std::cout << "}\n";
 }
 
 }
 
private: 
 // Computes the inner product between
 // vec1, vec2, both vectors of length.
 int innerProduct(int* vec1, int* vec2, int length) {
 int sum = 0;
 for (int i = 0; i < length; i++) {
 sum += vec1[i]*vec2[i];
 } 
 return sum;
 }
 
};
int main() {
 int data1[6] = {0, 1, 
 1, 0,
 2, 2};
 Matrix mat1(3, 2, data1);
 
 int data2[8] = {1, 2, 3, 4,
 5, 6, 7, 8};
 Matrix mat2(2, 4, data2);
 
 Matrix product = mat1.product(mat2);
 
 std::cout << "Matrix 1 looks like\n";
 mat1.print();
 std::cout << "Matrix 2 looks like\n";
 mat2.print();
 std::cout << "The product looks like \n";
 product.print();
}

• Reinderien: As a learning exercise, fine; but don't do this in real life - you'll be outperformed by any self-respecting BLAS library.
• Loki Astari: Separate business logic from resource management. i.e. Use one class to handle resource management only (i.e. memory management) and another class to handle the business logic (matrix stuff). Mixing the two just makes things harder to validate. Also separating them allows you to check them independently. You could write your matrix class using std::vector to handle the resource management first. Then when the matrix code works swap in an alternative resource management class that you have written.

Answer 1

Prefer std::vector to Manually Allocation of Memory

I can see you are manually handling your memory, this is good but also opens the door wide for bugs to creep in. STL container std::vector is your best bet here. It really makes things easier for you. It gives you the following for free:

1. Copy constructor
2. Copy assignment
3. Move constructor
4. Move assignment
5. Destructor

This is a huge favor, remember less code is good code A common definition might look like this

template<typename T>
class Matrix
{
 public:
 Matrix(size_t row, size_t column, const std::vector<T>& data)
 {
 if((row * column) != data.size())
 throw std::invalid_argument("Data size does not match the dimension"); 
 row_ = row;
 column_ = column;
 data_ = data;
 
 }
 Matrix(size_t row, size_t column, const T& val) 
 : row_{row}, column{column}, data_{std::vector<T>(row * column, val)}{ }
 private:
 size_t row_;
 size_t column_;
 std::vector<T> data_;
};

The extra check guarantees that users of our class do not construct ill-formed matrix objects. So we are guaranteed that our class satisfies the invariant that row * column == data.size()

Prefer Operator Overloading for Basic Functionalities

C++ users are accustomed overloaded operators such as operator+, operator(), operator<<. This reduces the need of learning new method, when possible prefer function overloading to defining your own method function. So we can redefine getElement as follow

T& operator()(const size_t i, const size_t j)
{
 return data_.at(column_ * i + j);
}

With this you get an extra out of range checks for free. You could overload operator() that takes one argument here to display a row or column but I would choose to not overload in this scenario because it might raise confusion.

#include <algorithm>
std::vector<T> get_row(const size_t i)
{
 if(row_ <= i)
 throw std::invalid_argument("Invalid row dimension");
 size_t start_index = column_ * i;
 size_t end_index = start_index + column_;
 std::vector<T> vec_(end_index - start_index);
 copy(data_.begin()+start_index, data_.begin()+end_index, vec_.begin());
 return vec_;
}

As you would notice, there are extra checks for out of bound access. Same approach can be used for get_column also but with little changes.

Prefer pass by reference to pass by value

Most if not all of your method receives arguments by value, this can get expensive when passing maybe a 1000 x 1000 matrix, you always should prefer pass by reference and if the object is not allowed to change, pass by const reference. This avoids the copy.

Other Points to consider

In addition, your code breaks if we try to add unequal matrix objects, perform a check to guarantee this invariant before proceeding with addition.

delete sum, does this actually delete our dynamic array? of course not, this is the more reason you should prefer std::vector and even if you want to manage memory, prefer std::unique_ptr to raw new. A proper way to delete a dynamically allocated array is

delete [] sum

Method function product also does not yield expected results, you can only compute product of a matrix that satisfies the invariant that A.row == B.column where A and B are matrix objects. Doing otherwise produces unexpected results. So we could rewrite this to be

Matrix<T> operator*(const Matrix& rhs)
{
 if(row_ != rhs.column_ )
 throw std::runtime_error("Unequal size in Matrix*");
 Matrix res(rhs.row_, column_, 0.0);
 for(int i = 0; i != rhs.row_; ++i)
 {
 for(int j = 0; j != column_; ++j)
 {
 for(int k = 0; k != row_; ++k)
 {
 res(i, j) += rhs(i, k) * this->operator()(k, j);
 }
 }
 }
 return res;
}

I would skip addition because it is trivial. You could also implement inplace product, inplace addition both for scalar and matrix.

I know we all love a nice print member function, but C++ programmers expect to see their objects when they use the standard output stream and might forget to call your output display function; overload std::ostream for output functionalities. A typical definition is

std::ostream& operator<<(std::ostream& os, const Matrix<int>& mat)
{
 for (int i = 0; i != mat.row_; ++i) {
 os << '{';
 for (int j = 0; j != mat.column_; ++j) {
 os << mat(i, j) << ", ";
 }
 os << "}\n";
 }
 return os;
}

Note that you have to declare the function as a friend to Matrix class in order to access the private member data, if you don't want this behaviours, create public method and access the data via the public methods.

• indi: You should probably be more specific about where to pass by ref. In addition() and product(), sure, pass by ref... but in the constructor, passing by value is the right way to do it.
• Toby Speight: I think it would be better if your Matrix constructor accepted the vector by value and std::move() it to the data member. And also better to use initialisers rather than assignment. +1 for the rest of the review!
• Mark H: Your constructor size check should use !=.
• indi: In that constructor you wrote, taking the vector by const& is not the right way to do it, because it is a sink function. You end up needlessly copying the entire vector. The right way to do it is take the vector by value, and then move it into data_. Generally if you’re reading a value, you take it by const&... if you’re taking a value (taking it to keep it, rather than just reading it to use it), you should take it by value.
• Toby Speight: Best practice: Matrix(std::size_t width, std::size_t height, std::vector<T> data) : width{width}, height{height}, data{std::move(data)} { if (width * height != this->data.size()) throw std::invalid_argument("Data size does not match the dimension"); } - that doesn't re-assign the vector after initialisation, and doesn't needlessly copy when data is provided as rvalue (e.g. as in Matrix identity{2, 2, { 1, 0, 0, 1 }}).

Answer 2

Summary

Your resource management is broken.
You have not implemented the rule of 3 or 5 and so you are going to leak and/or do double deletes. So let's concentrate on the Matrix part rather than resource management.

You can replace your resource management with std::vector<> for now. In the future you may want to replace that with your own memory management if you have something specific you want to try and achieve but std::vector should work fine for now.

Design Issues

1. Copies with no ownership semantics.
2. Only Integers (not floats). You could template this to allow other types.
3. Using methods were operators would be more logical.
4. General resource management failures.

Code Review.

I don't see any use of std::string in the code!

#include <string>

---

Sure you could initialize from a C-array (pointer) but we don't know the size of the underlying array, so we can't check and make sure that our reads are valid.

It is more traditional to use Iterators in C++ as we know where the end of the data is. Also it allows us to abstract the underlying container (it could be array/vector/list etc...)

 Matrix(int m, int n, int* data) {

---

Normally you want to use the initializer list to set up member variables:

 Matrix(int m, int n, int* data) {
 rows = m;
 columns = n;
 contents = new int[m*n];

I would have written like this:

 Matrix(int m, int n, int* data)
 : rows{m}
 , columns{n}
 , contents{new int[m*n]}
 {

In this case it does not make a difference. BUT if the members are objects and have constructors your technique adds extra work (as you default construct then call the assignment operator). By using the initializer list you always initialize exactly once using the constructor.

So the best practice is to prefer the initializer list.

---

Sure you can use a loop.

 for (int i = 0; i < m*n; i++) {
 contents[i] = data[i]; 
 }

But we have standard algorithms for replacing simple loops like this:

 std::copy(std::begin(data), std::end(data), std::begin(contents));

---

Sure.

 int getElement(int i, int j) {
 return contents[columns*i + j];
 }

But you can overload operator[] or operator() to make the code read more naturally for matrix operations.

This example reads more naturally to me:

 Matrix m(2,3,data);
 std::cout << m[1][2] << "\n";

---

You are returning a copy of the value here. Not a big deal for int as this is not expensive. But it would be nice to allow people to overwrite the member in the array:

 m[1][2] = 3; // Write a value into the vector.

To achieve this you need to return reference to the values inside the matrix.

 // translating to your API:
 int& getElement(int i, int j);
 ^^^^ Return value by reference.
 // Also add a const version to allow usage in a constant context.
 // i.e. when you pass the matrix by const reference
 int const& getElement(int i, int j) const; // return const ref
 // mark method const

---

This has some major issues

 int* getRow(int i)

You are returning a dynamically allocated object here. This means you are returning ownership but pointers int* does not indicate ownership transfer so you are not sure if you are returning ownership.

If you continued implementation in the same way you can indicate transfer of ownership using smart pointers.

 std::unique_ptr<int[]> getRow(int i)

Now I know I am being given something I own and it will be auto cleaned up when I am finished using it.

---

But having said that, this is probably not the best way to implement this as you are making a copy. Might be better to create an object that refers to the data in place.

 class RefMatrix
 {
 Matrix& parent;
 int offset;
 int skip;
 public:
 Row(Matrix& parent, int offset, int skip)
 : parent(parent)
 , offset(offset)
 , skip(skip)
 {}
 int& operator[](int i) {
 return parent.data[offset + skip * i];
 }
 };
 RefMatrix getRow(int i) {return RefMatrix(*this, i , columns);}

This returns an object that accesses the raw data in place in the original matrix without having to dynamically create an object and without having to copy.

---

Like access, you can override appropriate methods:

 Matrix addition(Matrix other) {

I would implement operator+ and operator+= to do this function.

---

This is a bit inefficient. You allocate memory to calculate the result, then copy the data into the array which does another allocation.

 int* sum = new int[rows*columns];
 
 for (int i = 0; i < rows*columns; i++) {
 sum[i] = contents[i] + other.contents[i]; 
 }
 
 Matrix sumMatrix(rows, columns, sum);
 
 delete sum;

We can do this a lot more simply:

 Matrix sumMatrix(rows, columns, data); // Make a copy.
 for (int i = 0; i < rows*columns; i++) { // Now add the data into it
 sumMatrix.data[i]+= other.contents[i]; 
 }
 
 return sumMatrix;
 }

---

Sure: Though I would make the function const, as the function does not change the state of the object.

 void print() { 
 
 for (int i = 0; i < rows; i++) {
 std::cout << '{';
 for (int j = 0; j < columns; j++) {
 std::cout << getElement(i, j) << ", ";
 }
 std::cout << "}\n";
 }
 
 }

I would also add an override for operator<< as this is the standard way to implement streaming in C++.

 friend std::ostream& operator<<(std::ostream& str, Matrix const& data)
 {
 data.print(str); // would need to modify print to take a stream
 return str;
 }
 void print(std::ostream& str = std::cout) const; // this would work.

• Loki Astari: @TobySpeight Typo. Meant m[1][2] Easy way to implement. stackoverflow.com/a/1971207/14065
• Toby Speight: I don't have a strong personal position, but there are many who argue for overloading operator()(std::size_t, std::size_t) rather than a proxy row object. I can't remember the specific arguments, but it might be worth mentioning that there are pros and cons on both sides? I think the real problem with row views is being clear about object lifetimes - but perhaps with increasing use of view types in modern C++, we're more used to awareness of that?
• Toby Speight: An alternative is to use a Coordinate type as index.