patterncppMinor
Matrix template with basic operations and utilities
Viewed 0 times
templateoperationswithutilitiesandmatrixbasic
Problem
I implemented a basic
Before you start reading it, please look at some rationale about horrible aspects of 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
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()andcolumns()was thatrowCountandcolumnCountwere 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 onceis 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:
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
Another bug
Once again, in the multiplication method you have:
Unfortunately, that is not enough. You must swap column and row counts as well:
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:
since you write
size()
Too much code. Just leave
Forgot iostream
Your
Since your
Actually, each header file should sustain itself and include everything required to run the code in the header, so I suggest you include
Immutability of vectors
Unfortunately, this allows editing the matrix rows. Consider removing it.
Alternative implementation
You could use a single
(Also, const version as well.)
The above will exclude the possibility of messing around with the internals.
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 writeT& 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.