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

Matrix template with basic operations and utilities

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

Problem

I implemented a basic Matrix class for additions of matrices, multiplication of them, and multiplying every entry of matrix by constant, operator== and operator!=, overloads for I/O operations using istream and ostream.

Before you start reading it, please look at some rationale about horrible aspects of it:

  • The reason for naming member functions rows() and columns() was that rowCount and columnCount were already reserved for private member variables, thus I couldn't name them that way. I was thinking between just returning the dimensions from vector itself, but chose the way the code is.



  • My knowledge about const member functions is not sufficient to be confident, that is why I made the code as messy as it is, but I hope it is safe, at least.



  • I think that range loops are more explicit and names for the variables are clear enough, but I would like to receive an opinion of you.



  • According to wikipedia page, #pragma once is supported wide enough to still use it.



Here is the header:

```
#pragma once

#include
#include
#include

template
class Matrix
{
std::vector> matrix;
size_t rowCount;
size_t columnCount;
public:
Matrix(size_t rowCount_, size_t columnCount_):
matrix(rowCount_),
rowCount(rowCount_),
columnCount(columnCount_)
{
for (auto& row : matrix)
{
row.resize(columnCount);
for (auto& cell : row)
{
cell = 0;
}
}
}

Matrix(size_t rowCount_, size_t columnCount_, const T& value) :
matrix(rowCount_),
rowCount(rowCount_),
columnCount(columnCount_)
{
for (auto& row : matrix)
{
row.resize(columnCount, value);
}
}

Matrix(const Matrix& other) = default;

Matrix(Matrix&& other) :
matrix(std::move(other.matrix))
{
rowCount = other.rowCount;
columnCount = other.columnCount;
}

Matrix& oper

Solution

A bug

Your multiplication algorithm has a bug:

temp[i][j] += matrix[i][k] * rhs[j][k];


Your version will produce

$$
\begin{bmatrix}
1 & 2 \\
3 & 4
\end{bmatrix}
\begin{bmatrix}
2 & 3 \\
4 & 5
\end{bmatrix}
=
\begin{bmatrix}
8 & 14 \\
18 & 32
\end{bmatrix},
$$
when the right result is
$$
\begin{bmatrix}
10 & 13 \\
22 & 29
\end{bmatrix}.
$$
You actuall need

temp[i][j] += matrix[i][k] * rhs[k][j]; // Swap k and j in rhs.


Another bug

Once again, in the multiplication method you have:

std::swap(matrix, temp.matrix);


Unfortunately, that is not enough. You must swap column and row counts as well:

std::swap(matrix, temp.matrix);
std::swap(columnCount, temp.columnCount);
std::swap(rowCount, temp.rowCount);


In order to replicate the bug, compute

$$
\begin{bmatrix}
3 \\
4
\end{bmatrix}
\begin{bmatrix}
1 & 2 \\
\end{bmatrix}.
$$
Your version will return

$$
\begin{bmatrix}
3 \\
4
\end{bmatrix}
$$
instead of correct
$$
\begin{bmatrix}
3 & 6 \\
4 & 8
\end{bmatrix}.
$$

Edit Actually, you need to swap only the column count:

std::swap(matrix, temp.matrix);
std::swap(columnCount, temp.columnCount);


since you write

Matrix temp(rowCount, rhs.columnCount);


size()

size_t rows()
{
    return rowCount;
}

size_t columns()
{
    return columnCount;
}

const size_t rows() const
{
    return rowCount;
}

const size_t columns() const
{
    return columnCount;
}


Too much code. Just leave

size_t columns() const 
{
    return columnCount;
}

size_t rows() const
{
    return rowCount;
}


Forgot iostream

Your matrix.h did not compile for me until I #included iostream.

Since your matrix.h does not include cin, you should include iostream in your main.cpp before matrix.h:

#include 
#include "matrix.h"


Actually, each header file should sustain itself and include everything required to run the code in the header, so I suggest you include iostream in matrix.h.

Immutability of vectors

std::vector& operator[](size_t row)
{
    return matrix[row];
}


Unfortunately, this allows editing the matrix rows. Consider removing it.

Alternative implementation

You could use a single vector of size columns * rows. Then, you could write

T& Matrix::at(int row, int col) 
{
    // Check the validity of indices here.
    return storage_vector.at(row * columnCount + col);
}


(Also, const version as well.)

The above will exclude the possibility of messing around with the internals.

Code Snippets

temp[i][j] += matrix[i][k] * rhs[j][k];
temp[i][j] += matrix[i][k] * rhs[k][j]; // Swap k and j in rhs.
std::swap(matrix, temp.matrix);
std::swap(matrix, temp.matrix);
std::swap(columnCount, temp.columnCount);
std::swap(rowCount, temp.rowCount);
std::swap(matrix, temp.matrix);
std::swap(columnCount, temp.columnCount);

Context

StackExchange Code Review Q#126534, answer score: 8

Revisions (0)

No revisions yet.