eccentricorange / c-cpp-mirror Goto Github PK

Inside the declaration of class Matrix you don't need to repeat the template parameters if they are the same as the object's, you can just write Matrix and it will be equivalent to Matrix<R, C>. Prefer the short form, there is less chance of making mistakes this way, and if you really do want different template parameters you can still do that, but it will then clearly stand out.

Inheritance is one way to create a SquareMatrix class. However, it has some drawbacks, and there are alternatives you could use. Consider for example making getDeterminant() an out-of-class function that takes a SquareMatrix as a parameter. Now I can write:

SquareMatrix<…> square;
auto det = getDeterminant(square);

But now consider I wrote:

Matrix<3, 3> square;
auto det = getDeterminant(square); // compile error

This fails to compile because getDeterminant() only works on SquareMatrixes, even though Matrix<3, 3> is also square. You could create an overload for getDeterminant() to take square Matrix objects as well, but that's more work and extra code, with more chance of bugs.

Instead of using inheritance, consider creating a type alias:

template<std::size_t N>
using SquareMatrix = Matrix<N, N>;

Your getDeterminant() function will then look like:

template<std::size_t N>
double getDeterminant(const SquareMatrix<N>& matrix) {…}

But SquareMatrix was just an alias, so I can pass in a Matrix<3, 3> and it will still work. If I pass in a Matrix<4, 5> it will fail to compile.

Another option would be to use static_assert(), SFINAE or C++20's concepts to ensure getDeterminant() only compiles for square matrices. For example:

template<std::size_t R, std::size_t C>
double getDeterminant(const Matrix<R, C>& matrix) {
    static_assert(R == C, "The determinant is only defined for square matrices.");
    …
}

Or with C++20:

template<std::size_t R, std::size_t C>
requires (R == C)
double getDeterminant(const Matrix<R, C>& matrix) {
    …
}

In this simple case you can also just make sure template deduction only works for square matrices:

template<std::size_t N>
double getDeterminant(const Matrix<N, N>& matrix) {…}

But in fact I already showed that, because this is equivalent to writing getDeterminant(const SquareMatrix<N>& matrix).

Instead of writing the operators as class member functions, you can also make them out-of-class friend functions:

template<…>
class Matrix {
    …
    friend Matrix operator+(const Matrix& lhs, const Matrix& rhs);
    …
};

template<std::size_t R, std::size_t C>
Matrix<R, C> operator+(const Matrix<R, C>& lhs, const Matrix<R, C>& rhs) {
    Matrix<R, C> result;
    …
        result[row][column] = lhs[row][column] + rhs[row][column];
    …
    return result;
}

You might not see the point of this, but it has several advantages. In particular, consider that you might want to allow multiplying a matrix by a scalar value. You could add an overload of the member function operator*():

Matrix operator*(double value) {…}

That would allow you to write:

Matrix<…> matrix(…);
auto matrix2 = matrix * 2;

But it would not allow you to write:

auto matrix2 = 2 * matrix;

And there is no way to achieve that with a member function. However, the friend operators get two parameters, so then you can do:

friend Matrix operator*(const Matrix& lhs, double rhs);
friend Matrix operator*(double lhs, const Matrix& rhs);

In this case you can easily define one overload in terms of the other in order to avoid code duplication:

template<std::size_t R, std::size_t C>
friend Matrix<R, C> operator*(double lhs, const Matrix<R, C>& rhs) {
    return rhs * lhs;
}

Apart from the binary operators themselves, you can also make members like getDeterminant() friends instead:

friend double getDeterminant(const Matrix& matrix);

You would then need to call it like getDeterminant(matrix) instead of matrix.getDeterminant(), but that's similar to how you use std::abs() and other standard math functions.

Wouldn't it be nice if you could print by write the following?

Matrix<…> matrix(…);

std::cout << matrix;

You can quite easily change your printMatrix() function into an overload of operator<<(std::ostream&). The main advantage is that you no longer are hardcoding where the output goes: you can then print to std::cerr, to a std::stringstream, a std::fstream and whatever else provides a std::ostream interface.

Good feedback on determinant-inheritance.cpp has been recieved at https://codereview.stackexchange.com/a/285557/257004. This should be included in the code.

• #2
• #3
• #4
• #5
• #6
• #7
• #8
• #9
• #10

I see you are using printf() in your code. Consider avoiding this and using the C++ way of printing instead. The main disadvantage of printf() is that it is not type safe. The main advantage of printf() is perhaps the format string, but since C++20 there is std::format(), and in C++23 we will get the best of everything with std::print(). If you cannot use a newer C++ version yet, you can link with the {fmt} library to get the same functionality.

Note that with the "old" way of printing in C++, you could have written:

void printMatrix() {
    std::cout << "\n[" << R << 'x' << C << "] matrix\n";

    for (auto& row: matrix) {
        for (auto& value: row) {
            std::cout << std::setprecision(0) << value << '\t';
        }

        std::cout << '\n';
    }

    std::cout << '\n';
}

Note that if you want to keep using C's printf(), you should #include <cstdio> instead of #include <iostream>.

Your operator*() only allows two matrices of the same size to be multiplied. However, in mathematics, you can multiply two matrices of different size, as long as the number of columns on the left hand matches the number of rows on the right hand of the multiplication. You can easily change your operator to do this. First, you know that the matrix on the right hand side should have the same number of rows as the left hand side has columns, that's just C. But then the right hand side can have it's own number of columns, so let's name that RHC. Then you can just write:

template<std::size_t RHC>
Matrix<R, RHC> operator*(const Matrix<C, RHC>& matrix) {
    …
}

And you just have to change one of the for-loops to use RHC.

While your code might be functionally correct, it makes copies of matrices in lots of places. For example, because operator+() takes the parameter matrix by value, whenever it is called a copy is made of that matrix. To avoid that, pass the input by const reference:

Matrix operator+(const Matrix& matrix) {
    …
}

You should not do the same for the return value; copy elision will take of that.

You have getElement() and setElement() members, but have you noticed that standard containers like std::array and std::vector don't have such functions at all? They have overloads of operator[] and have a member function at() that allows read/write access to a given element. You can do the same:

double& at(std::size_t row, std::size_t column) {
    return matrix[row][column];
}

Because a reference is returned, the caller can then do:

matrix.at(1, 2) = 3;

Also note that your getElement() returned an int instead of a double.

You can also overload operator[]. Before C++23, it was not possible to pass more than one parameter inside the brackets, but you could have cheated by writing something like:

double& operator[](std::pair<std::size_t, std::size_t> index) {
    return matrix[index.first][index.second];
}

And call it like:

matrix[{1, 2}] = 3;

Member functions themselves can be marked const to indicate that they won't modify any member variables. Consider writing:

const Matrix<…> matrix(…);
auto value = matrix.getElement(1, 2); // compile error!

This wouldn't compile, because getElement() is not marked const, so the compiler will not allow you to call it on a const object. The simple solution is to make getElement() const:

double getElement(std::size_t row, std::size_t column) const {
   …
}

But that works here because this function returns by value, and never has to modify any members. But the above at() has a problem: if you want to be able to use it to write to an element, it needs to return a non-const reference. But that is not allowed if you apply it to a const object. You can however create a second overload for const objects:

double& at(std::size_t row, std::size_t column) {
    return matrix[row][column];
}

const double& at(std::size_t row, std::size_t column) const {
    return matrix[row][column];
}

You'll note that the body of those functions are the same, which is annoying. There are two ways around it; having one call the other but using std::const_cast to cheat. Another way is to use C++23's explicit object parameter.