HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Mathematical matrices implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationmathematicalmatrices

Problem

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 

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 
#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 m != m2->m or m1->n != m2->n) {
        cerr m, m1->n);

    for (int i = 0; i m; i++) {
        for (int j = 0; j 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
#include "matrix.h"

using namespace std;

int main() {

// --- Test code ---
int m, n;
Matrix* mat1;
Matrix* mat2;
Matrix* mat3;

cout > m;
cout > 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 m; i++) {
for (int j = 0; j n; j++) {
cout get(i,j);
}
cout << endl;
}

Solution

-
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.

-
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 m; i++) {
    for (int j = 0; j n; j++) {
        res->array[i][j] = m1->array[i][j] + m2->array[i][j];
    }
}


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

-
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).

-
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> array;
     ...
 };


It allows you to avoid manual memory management issues.

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

-
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++).

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

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

Code Snippets

~Matrix() {
    for (int row = 0; row < n; row++)
        delete[] array[row];
    delete[] array;
}
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];
    }
}
class Matrix {
     std::vector<std::vector<int>> array;
     ...
 };
class Matrix {
    ...
public:
    Matrix operator+(const Matrix& rhs) const {
        // Check the validity of input parameters.
        Matrix res(n, m);
        // Addition goes here.
        return res;
    }

    ...
};

Context

StackExchange Code Review Q#81751, answer score: 3

Revisions (0)

No revisions yet.