12
\$\begingroup\$

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:

  1. 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.
  2. 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.
  3. 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.
  4. According to wikipedia page, #pragma once is supported wide enough to still use it.

Here is the header:

#pragma once

#include <vector>
#include <exception>
#include <iosfwd>

template <typename T>
class Matrix
{
    std::vector<std::vector<T>> 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& operator=(const Matrix& other) = default;

    Matrix& operator=(Matrix&& other)
    {
        std::swap(matrix, other.matrix);
        rowCount = other.rowCount;
        columnCount = other.columnCount;
        return *this;
    }

    Matrix& operator*=(const T& rhs)
    {
        for (auto& row : matrix)
        {
            for (auto& cell : row)
            {
                cell *= rhs;
            }
        }
        return *this;
    }

    Matrix& operator*=(const Matrix& rhs)
    {
        if (columnCount != rhs.rowCount)
        {
            throw std::logic_error("column count of lhs and row count of rhs are not equal\n");
        }

        Matrix temp(rowCount, rhs.columnCount);

        for (size_t i = 0; i < temp.rowCount; i++)
        {
            for (size_t j = 0; j < temp.columnCount; j++)
            {
                for (size_t k = 0; k < columnCount; k++)
                {
                    temp[i][j] += matrix[i][k] * rhs[j][k];
                }
            }
        }
        std::swap(matrix, temp.matrix);

        return *this;
    }

    Matrix& operator+=(const Matrix& rhs)
    {
        if (rowCount != rhs.rowCount || columnCount != rhs.columnCount)
        {
            throw std::logic_error("either or both of row count and column count of lhs and rhs are not equal\n");
        }

        for (size_t i = 0; i < rowCount; i++)
        {
            for (size_t j = 0; j < columnCount; j++)
            {
                matrix[i][j] += rhs[i][j];
            }
        }

        return *this;
    }
    size_t rows()
    {
        return rowCount;
    }

    size_t columns()
    {
        return columnCount;
    }

    const size_t rows() const
    {
        return rowCount;
    }

    const size_t columns() const
    {
        return columnCount;
    }

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

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

template <typename T>
bool operator==(const Matrix<T>& lhs, const Matrix<T>& rhs)
{
    if (lhs.rows() != rhs.rows() || lhs.columns() != rhs.columns())
    {
        return false;
    }

    for (int i = 0; i < lhs.rows(); i++)
    {
        for (int j = 0; j < lhs.columns(); j++)
        {
            if (lhs[i][j] != rhs[i][j])
            {
                return false;
            }
        }
    }
    return true;
}

template <typename T>
bool operator!=(const Matrix<T>& lhs, const Matrix<T>& rhs)
{
    return !(lhs == rhs);
}

template <typename T>
Matrix<T> operator+(Matrix<T> lhs, const Matrix<T>& rhs)
{
    return lhs += rhs;
}

template <typename T>
Matrix<T> operator*(Matrix<T> lhs, const Matrix<T>& rhs)
{
    return lhs *= rhs;
}

template <typename T>
Matrix<T> operator*(Matrix<T> lhs, const T& rhs)
{
    return lhs *= rhs;
}

template <typename T>
Matrix<T> operator*(const T& lhs, Matrix<T> rhs)
{
    return rhs *= lhs;
}

template <typename T>
std::istream& operator >> (std::istream& is, Matrix<T>& matrix)
{
    for (size_t i = 0; i < matrix.rows(); i++)
    {
        for (size_t j = 0; j < matrix.columns(); j++)
        {
            is >> matrix[i][j];
        }
    }

    return is;
}

template <typename T>
std::ostream& operator<< (std::ostream& os, const Matrix<T>& matrix)
{
    for (size_t i = 0; i < matrix.rows(); i++)
    {
        for (size_t j = 0; j < matrix.columns(); j++)
        {
            os << matrix[i][j] << ' ';
        }
        os << "\n";
    }

    return os;
}

Example usage:

#include "matrix.h"
#include <iostream>

int main()
{
    std::cout << "Enter the dimensions of the first matrix (non negative): ";
    size_t rowCount = 0, columntCount = 0;
    std::cin >> rowCount >> columntCount;
    Matrix<double> firstMatrix(rowCount, columntCount);
    std::cout << "Enter values of the matrix entries\n";
    std::cin >> firstMatrix;

    std::cout << "Enter the dimensions of the second matrix (non negative): ";
    std::cin >> rowCount >> columntCount;
    Matrix<double> secondMatrix(rowCount, columntCount);
    std::cout << "Enter values of the matrix entries\n";
    std::cin >> secondMatrix;

    std::cout << "Choose operator (+ or *): ";
    char op;
    std::cin >> op;

    switch (op)
    {
    case '*':
        std::cout << firstMatrix * secondMatrix;
    default:
        std::cout << firstMatrix + secondMatrix;
    }

    std::system("pause");
}
\$\endgroup\$
0

3 Answers 3

8
+100
\$\begingroup\$

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 <iostream>
#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<T>& 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<T>::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.

\$\endgroup\$
2
  • \$\begingroup\$ About iostreams, it is actually intended! <iosfwd> only declares std::ostream so that it will increase compilation time and user will include their stream if needed, otherwise it won't bring whole iostream. Otherwise, thanks for the review! \$\endgroup\$ Commented Jan 3, 2017 at 8:07
  • \$\begingroup\$ Sorry, I overlooked it. Thanks for noticing another bug. \$\endgroup\$ Commented Jan 3, 2017 at 8:38
2
\$\begingroup\$

Well, coderodde already found the bugs, and he's right that you should consider mapping the matrix onto a single piece for compactness.
Still, the underlying storage should be provided by a std::unique_ptr<T[]> instead of a std::vector<T> to avoid redundancy.

The ctor Matrix(size_t rowCount_, size_t columnCount_) should mirror or be implemented in terms of the ctor Matrix(size_t rowCount_, size_t columnCount_, const T& value). The innermost loop is redundant.

The move-ctor should construct a 0/0-sized matrix and then swap everything. There's no appreciable benefit to allowing a zombie-matrix violating matrix's invariants.
Same for move-assignment, make it a full swap.

By the way, where is the non-member swap-function? A custom function can be more efficient and helps in implementing the rest.

The subclass std::domain_error seems to be a better description of the error when the dimensions are wrong for some operation than std::logic_error.

I would reserve rows and columns for a range-object for iterating over them, and use row_count and column_count for obtaining the counts.

The problem with your mutable operator[] is that a user can change the size of the returned row-vector, which is invalid. That's a problem of the representation.

You are missing operator*(Matrix<T> rhs, const T& lhs).

Your stream-extractor can only extract matrices of known dimensions. That's somewhat inevitable given the format your stream-inserter produces.

You should try to consistently insert single characters as characters instead of strings for efficiency, even if they are written as an escape-sequence.

\$\endgroup\$
1
\$\begingroup\$

You've already got some good answers, so I'm only going to address one of your design decisions:

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.

This decision resulted in a rows method that consists of a single line, returning the rowCount field.

size_t rows()
{
    return rowCount;
}

Without any context, this looks confusing. There are two things to consider here.

Use a naming convention to avoid conflicts

There are various naming conventions that can be used to give variables names. They each have their own followers, benefits and drawbacks. If you use different conventions such as 'snake_case' for member variables and 'camelCase' for method names then it can help to avoid conflicts between them. Alternately, some places will prefix member variables with a known value e.g. 'm_rowCount', or be explicit about get methods 'get_rowCount'. Find a convention that works for you and adopt it.

If you can't work around a conflict, then:

Prioritise the public interface

Your classes public interface is the one that is used by external clients. In many ways it is more important than the private implementation. If you decide that you want to rename a private field, you only have to fix any references to the field in that class. If you decide to rename a public method on the class, you may have to refactor many dependent pieces of code to take account of the rename. If you have to make a decision about which name to get right, make it the public one.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.