Mathematical matrices implementation

Question

As a Java programmer who's trying his hand at C++, I thought I'd start building a library for working with matrices as my first C++ project. I've got very little C++ experience and it's been a long time since I've even so much as dabbled in it.

So far I've only implemented an addition function, but I think this should be enough code to establish my current coding style.

I was wondering if anyone would be able to give my code a look over and see if it conforms to standards and if there are any cleaner ways of doing anything.

Here is my matrix.h file:

#ifndef MATRIX_H
#define MATRIX_H
#include <iostream>
using namespace std;
class Matrix {
 private:
 int** array;
 public:
 int m, n;
 Matrix(int m, int n);
 static Matrix* add(Matrix* m1, Matrix* m2);
 int get(int i, int j);
 void set (int i, int j, int e);
 ~Matrix();
};
#endif

and my matrix.cpp file:

#include <iostream>
#include "matrix.h"
using namespace std;
Matrix::Matrix(int var_m, int var_n) {
 m = var_m;
 n = var_n;
 array = new int*[n];
 for (int i = 0; i < n; i++)
 array[i] = new int[m];
 cout << "foo" << endl;
 cout << m << " " << n << endl;
}
Matrix* Matrix::add(Matrix* m1, Matrix* m2) {
 if (m1->m != m2->m or m1->n != m2->n) {
 cerr << "Error: Mismatched sizes in Matrix.add()" << endl;
 throw;
 }
 Matrix* res = new Matrix(m1->m, m1->n);
 for (int i = 0; i < res->m; i++) {
 for (int j = 0; j < res->n; j++) {
 res->array[i][j] = m1->array[i][j] + m2->array[i][j];
 }
 }
 return res;
}
int Matrix::get(int i, int j) {
 return array[i][j];
}
void Matrix::set(int i, int j, int e) {
 array[i][j] = e;
}
Matrix::~Matrix() {
 delete array;
}

and just some test code thrown together in my main.cpp:

#include <iostream>
#include "matrix.h"
using namespace std;
int main() {
 // --- Test code ---
 int m, n;
 Matrix* mat1;
 Matrix* mat2;
 Matrix* mat3;
 cout << "m: ";
 cin >> m;
 cout << "n: ";
 cin >> n;
 mat1 = new Matrix(m, n);
 mat2 = new Matrix(m, n);
 mat1->set(0,2,5);
 mat3 = Matrix::add(mat1, mat2);
 for (int i = 0; i < mat3->m; i++) {
 for (int j = 0; j < mat3->n; j++) {
 cout << mat3->get(i,j);
 }
 cout << endl;
 }
 delete mat1, mat2, mat3;
 return 0;
}

Answer 1

1. There is a bug in your Matrix destructor. delete array is not correct. It was allocated using an array form of the new operator, so it must be deleted using an array form of delete(that is, delete[]). The current version of your code invokes undefined behavior. But simply changing it to delete[] array is not enough to fix it. There is still a memory leak here. A correct version of the destructor should look this way:

   ~Matrix() {
    for (int row = 0; row < n; row++)
    delete[] array[row];
    delete[] array;
   }
   

   It is necessary because deleting an array of pointers does not delete the objects pointed by this pointers.

2. It looks like the size of the array is not correct, either. You allocate an array of pointers of size n, but then you use it as if it had a size m.

   array = new int*[n];
   for (int i = 0; i < n; i++)
    array[i] = new int[m];
   ...
   for (int i = 0; i < res->m; i++) {
    for (int j = 0; j < res->n; j++) {
    res->array[i][j] = m1->array[i][j] + m2->array[i][j];
    }
   }
   

   This code invokes undefined behavior in case n != m.

3. You should probably implement a copy constructor and a copy-assignment operator. A default copy constrcutor(and assignment operator) just copies the pointer, not the content of the Matrix(I'm not sure if you it is what you want).

4. A good way to get rid of many bugs in your code is to use standard containers instead of dynamic memory allocation with raw pointers. For example, you can use a std::vector to store the elements of the Matrix:

    class Matrix {
    std::vector<std::vector<int>> array;
    ...
    };
   

   It allows you to avoid manual memory management issues.

5. Using raw pointers and the new operator is not common in modern C++ in general.

6. C++ has operator overloading, and it is usually used in C++ programs. For example, matrix addition is a good candidate for overloading the + operator.

   class Matrix {
    ...
   public:
    Matrix operator+(const Matrix& rhs) const {
    // Check the validity of input parameters.
    Matrix res(n, m);
    // Addition goes here.
    return res;
    }
    ...
   };
   

   This example also shows a conventional way to pass arguments without copying them: by const reference(passing a pointer to them is not common in modern C++).

7. Member-functions that do not change the instance should have a const qualifier.

8. It is a good practice to write unit tests instead of ad-hoc testing(it is not C++ specific, of course).

Answer 2

• C++ offers operator overload feature. For element access it is customary to overload an operator() (function call):

  const int& Matrix::operator()(int i, int j) const;
  int& Matrix::operator()(int i, int j);
  

  instead of Java-style get and set, which leads to a somewhat better readability, e.g.

  m(1, 2) = 1; // cf m.set(1, 2, 1)
  int x = m(1, 2); // cf x = m.get(1, 2)
  

• Similarly, operator+() is preferred to an add method.

Answer 3

In addition to @kraskevich answer:

It is better to use linear array of size m*n and compute offsets when needed (offset = i + m*j). In this case you can use algorithms and iterators, i.e. add two matrices would be:

class Matrix {
 ...
 std::vector<int> arr;
 ...
 friend Matrix operator+(const Matrix& a, const Matrix& b);
 ...
};
Matrix operator+(const Matrix& a, const Matrix& b) {
 assert(a.m == b.m && a.n == b.n);
 Matrix result;
 result.reserve(a.size());
 std::transform(a.arr.begin(), a.arr.end(), b.arr.begin(), 
 std::back_inserter(result.arr), std::plus<int>());
 return result;
}

Also this approach is faster and can be better optimized (you use MADD and memory read instead of two memory reads)

Answer 4

Major objection:

Don't use

using namespace std;

in a .h file. In brings the entire std namespace into the scope of any file that #includes the .h file. If you need anything from the std namespace, just use them explicitly -- i.e. use std::vector instead of vector.

Use std::vector<std::vector<int>> for member variable.

I agree with @kraskevich that you should change the member variable to

std::vector<std::vector<int>> array;

This allows you to overload the array operator, [].

std::vector<int>& operator[](size_t index) 
{
 return array[index];
}

and the const version.

std::vector<int> const& operator[](size_t index) const
{
 return array[index];
}

Now, you can use the following syntax to access the elements of a matrix:

Matrix mat(10, 15);
mat[0][0] = 10;

That's more idiomatic than

Matrix mat(10, 15);
mat.set(0, 0, 10);

You can also use:

int x = mat[0][0];

instead of

int x = mat.get(0, 0);

Remove the member variables m and n

With a std::vector<std::vector<int>> as the member variable, you have access to m and n. There is no need for storing redundant data.

Argument types in the constructor

Since it makes no sense to have negative values of m or n as inputs to the constructor, I would make then an unsigned integral type.

Matrix(unsigned int m, unsigned int n);

or

Matrix(size_t m, size_t n);

Provide an iterator and functions to return iterators

typedef std::vector<std::vector<int>>::iterator iterator;
typedef std::vector<std::vector<int>>::const_iterator const_iterator;
iterator begin() { return array.begin(); }
const_iterator begin() const { return array.begin(); }
iterator end() { return array.end(); }
const_iterator end() const { return array.end(); }

Now, you can iterate over the elements of a Matrix using the range for construct:

for ( auto& row : mat )
 for ( auto element : row )
 cout << element;