Matrix template with a vector buffer

Question

I need some feedback, please. Goals to practice: templates, std, interfaces, return value, constructors and some idioms. People often suggest to use a library, but it's not the goal of this exercise, so please don't suggest those.

My worries about this code

correctness of returns by value\reference
correctness of iterator constructor
correctness of type casts

Thanks

mymatrix.h

#pragma once

#include <iostream>
#include <algorithm>
#include <vector>
#include <cassert>

template <typename T>
class MyMatrix
{
public:
    using value_type = T;
    using reference = value_type&;
    using const_reference = value_type const&;
    using iterator = typename std::vector<value_type>::iterator;
    using const_iterator = typename std::vector<value_type>::const_iterator;
    using size_type = std::size_t;

private:
    size_type m_rows;
    size_type m_cols;
    std::vector<value_type> m_buffer;

public:
    MyMatrix(size_type dimx = 3, size_type dimy = 3)
        : m_rows(dimx)
        , m_cols(dimy)
        , m_buffer(dimx * dimy)
    {}
    // Copy constructor
    MyMatrix(MyMatrix const& copy)
        : m_rows(copy.m_rows)
        , m_cols(copy.m_cols)
        , m_buffer(copy.m_buffer)
    {}
    // Move constructor
    MyMatrix(MyMatrix&& move) noexcept
    {
        *this = std::move(move);
    }
    explicit MyMatrix<T>(iterator begin, iterator end, size_type dimx, size_type dimy)
        : m_rows(dimx)
        , m_cols(dimy)
        , m_buffer(std::distance(begin, end))
    {
        std::copy(begin, end, m_buffer.begin());
    }
    // Copy assignment
    MyMatrix& operator=(MyMatrix const& copy)
    {
        // Copy and Swap idiom
        MyMatrix<value_type> tmp(copy);
        tmp.swap(*this);
        return *this;
    }
    // Move assignment
    MyMatrix& operator=(MyMatrix&& move) noexcept
    {
        move.swap(*this);
        return *this;
    }

    // Iterators
    iterator       begin() { return m_buffer.begin(); }
    const_iterator begin()  const { return m_buffer.begin(); }
    const_iterator cbegin() const { return begin(); }

    iterator       end() { return m_buffer.end(); }
    const_iterator end()    const { return m_buffer.end(); }
    const_iterator cend()   const { return end(); }

    // Access operators with validation
    reference operator()(const size_type x, const size_type y)
    {
        size_type index = m_cols * x + y;
        assert(index < m_buffer.size() && "Index is out of range");
        return m_buffer[index];
    }
    const_reference operator()(const size_type x, const size_type y) const
    {
        size_type index = m_cols * x + y;
        assert(index < m_buffer.size() && "Index is out of range");
        return m_buffer[index];
    }
    reference operator[](size_type index)
    {
        assert(index < m_buffer.size() && "Index is out of range");
        return m_buffer[index];
    }
    const_reference operator[](size_type index) const
    {
        assert(index < m_buffer.size() && "Index is out of range");
        return m_buffer[index];
    }

    // Mutating functions
    void ident()
    {
        assert(m_rows == m_cols && "Matrix must be square!");
        for (size_type x = 0; x < m_rows; ++x) {
            for (size_type y = 0; y < m_cols; ++y)
                m_buffer[m_cols * x + y] = static_cast<T>(x == y); // CORRECT ?
        }
    }
    void fill(value_type value)
    {
        std::fill(m_buffer.begin(), m_buffer.end(), value);
    }
    void fillRand()
    {
        std::generate(m_buffer.begin(), m_buffer.end(), []() {return std::rand() % 10; });
    }
    void swap(MyMatrix<value_type>& other) noexcept
    {
        using std::swap;
        swap(this->m_rows, other.m_rows);
        swap(this->m_cols, other.m_cols);
        swap(this->m_buffer, other.m_buffer);
    }

    // Inspecting functions
    size_type rows() const
    { return m_rows; }
    size_type cols() const
    { return m_cols; }

    template<class T> // linkage error without this!
    friend std::ostream& operator<<(std::ostream& out, MyMatrix<T> const& mtx);

    // Matrix mathematical operations
    MyMatrix  operator+(MyMatrix const& mtx) const
    {
        MyMatrix<T> result(*this);
        return result += mtx;
    }
    MyMatrix& operator+=(MyMatrix const& mtx)
    {
        assert(m_rows == mtx.m_rows || m_cols == mtx.m_cols && "Matrix dimension must be the same.");
        std::transform(m_buffer.begin(), m_buffer.end(), mtx.m_buffer.begin(), m_buffer.begin(), std::plus<>{});
        return *this;
    }
    MyMatrix  operator-(MyMatrix const& mtx) const
    {
        MyMatrix<T> result(*this);
        return result -= mtx;
    }
    MyMatrix& operator-=(MyMatrix const& mtx)
    {
        assert(m_rows == mtx.m_rows || m_cols == mtx.m_cols && "Matrix dimension must be the same.");
        std::transform(m_buffer.begin(), m_buffer.end(), mtx.m_buffer.begin(), m_buffer.begin(), std::minus<>{});

        return *this;
    }
    MyMatrix  operator*(MyMatrix const& mtx) const
    {
        MyMatrix<T> tmp(*this);
        return tmp *= mtx;
    }
    MyMatrix  operator*=(MyMatrix const& mtx)
    {
        assert(m_cols == mtx.m_rows && "Invalid Matrix demensions.");
        MyMatrix<value_type> result(m_rows, mtx.m_cols);

        for (size_type r = 0; r < m_rows; r++) {
            for (size_type c = 0; c < mtx.m_cols; c++) {
                for (size_type i = 0; i < m_cols; i++) {
                    result.m_buffer[mtx.m_cols * r + c] += m_buffer[m_cols * r + i] * mtx.m_buffer[mtx.m_cols * i + c];
                }
            }
        }
        return result;
    }

    // Comparision
    bool operator==(MyMatrix const& mtx) const noexcept
    {
        if (m_rows != mtx.m_rows || m_cols != mtx.m_cols)
            return false;

        std::for_each(m_buffer.begin(), m_buffer.end(), [&](const unsigned int i) { return m_buffer[i] != mtx.m_buffer[i]; });

        return true;
    }
    bool operator!=(MyMatrix const& mtx) const noexcept { return !(*this == mtx); }

    // Matrix scalar operations
    MyMatrix& operator+(const T& value)
    {
        std::transform(m_buffer.begin(), m_buffer.end(), m_buffer.begin(), [&value](const T index) {return index + value; });
        return *this;
    }
    MyMatrix& operator-(const T& value)
    {
        std::transform(m_buffer.begin(), m_buffer.end(), m_buffer.begin(), [&value](const T index) {return index - value; });
        return *this;
    }
    MyMatrix& operator*(const T& value)
    {
        std::transform(m_buffer.begin(), m_buffer.end(), m_buffer.begin(), [&value](T index) {return index * value; });
        return *this;
    }
    MyMatrix& operator/(const T& value)
    {
        std::transform(m_buffer.begin(), m_buffer.end(), m_buffer.begin(), [&value](T index) {return index / value; });
        return *this;
    }
};

template <typename T>
std::ostream& operator<<(std::ostream& out, MyMatrix<T> const& mtx)
{
    std::size_t rows = mtx.rows();
    std::size_t cols = mtx.cols();

    for (size_t i = 0; i < rows; i++) {
        for (size_t j = 0; j < cols; j++) {
            out << mtx(i, j) << ' ';
        }
        out << "\n";
    }
    return out;
}
template <typename T>
MyMatrix<T> transpose(MyMatrix<T> const& mtx)
{
    std::size_t rows = mtx.rows();
    std::size_t cols = mtx.cols();

    MyMatrix<T> result(cols, rows);

    for (std::size_t r = 0; r < rows * cols; r++) {
        std::size_t i = r / rows;
        std::size_t j = r % rows;
        result[r] = mtx[cols * j + i];
    }

    return result;
}
template <typename T>
MyMatrix<T> inverse(MyMatrix<T> const& mtx)
{
    MyMatrix<T> result(mtx);

    std::transform(result.begin(), result.end(), result.begin(), [](const T index) {return 1 / index; });

    return result;
}
template <typename T>
MyMatrix<T> symmetric(MyMatrix<T> const& mtx)
{
    assert(mtx.cols() == mtx.rows() && "Invalid Matrix demensions.");
    MyMatrix<T> result(mtx);
    return mtx * transpose(mtx);
}

tried compiling using clang 10.0.1 but there is an error: 127:20: error: declaration of 'T' shadows template parameter — Peter
– Peter, Commented Aug 14, 2020 at 18:58

Edward · Accepted Answer · 2020-08-14 22:15:00Z

Here are some things that may help you improve your code.

Use include guards

There should be an include guard in each .h file. That is, start the file with:

#ifndef MATRIX_H
#define MATRIX_H
// file contents go here
#endif // MATRIX_H

The use of #pragma once is a common extension, but it's not in the standard and thus represents at least a potential portability problem. See SF.8

Use `throw` rather than `assert`

The bounds checking your program does is good, but it should throw an exception rather than using assert to be compatible with STL containers.

Implement mathematical operations with templates

Consider the following code:

MyMatrix<float> fm;
MyMatrix<int> im;
fm += im;

This ought to work but does not because the current code requires both matrices to be of the same type. Better would be to write the function like this:

template<class U>
MyMatrix& operator+=(MyMatrix<U> const& mtx)
{
    if (m_rows != mtx.rows() || m_cols != mtx.cols()) 
        throw std::invalid_argument("Matrix dimension must be the same.");
    std::transform(m_buffer.begin(), m_buffer.end(), mtx.begin(), m_buffer.begin(), std::plus<>{});
    return *this;
}

Now it works with any pair of types for which std::plus<> is defined.

Implement mathematical operations as freestanding functions

Consider this code:

MyMatrix<float> fm;
auto doppel = fm + fm;

It should work but does not. Fix that by defining operator+ as a freestanding templated function:

template <typename T, typename U>
MyMatrix<T>  operator+(MyMatrix<T> one, MyMatrix<U> const& two) {
    return one += two;
}

Don't shadow parameters

The inserter function is currently defined like this:

template<class T> // linkage error without this!
friend std::ostream& operator<<(std::ostream& out, MyMatrix<T> const& mtx);

The problem with that is it's inside a template that also takes a class T and the compiler has no way to distinguish between them. Fortunately, it's a simple fix here, just use a different letter, such as U for this declaration.

Implement unary operators

The unary - and unary + operators are missing. The result is that this fails:

std::cout << -foo << "\n";

You could implement unary - like this:

MyMatrix operator-() const {
    MyMatrix result(*this);
    std::transform(result.begin(), result.end(), result.begin(), std::negate<>{});
    return result;
}

Fix the spelling errors

In some place the word "dimension" is spelled incorrectly. Since your code is mostly pretty nice, it's worth the extra step to eliminate spelling errors.

Implement operators with constant arguments

The operator+= is defined, but only for two objects of type MyMatrix. I would suggest implementing each of the operators so that the right side can be a constant. For example, this won't compile:

MyMatrix fm;
fm += 2;

A simple way to address that is by defining those versions:

template<class U>
MyMatrix& operator+=(U const& val)
{
    std::for_each(m_buffer.begin(), m_buffer.end(), [val](T& item){ item += val; });
    return *this;
}

Eliminate work

The operator== is much more complex than it needs to be. Since the code is using a std::vector as the underlying storage, we can use the overloaded operator== for that and simplify the code:

bool operator==(MyMatrix const& mtx) const noexcept
{
    return m_rows == mtx.m_rows && m_cols == mtx.m_cols && m_buffer == mtx.m_buffer;
}

Implement a `size()` operator

I'd write one like this:

size_type size() const 
{ return m_buffer.size(); }

Avoid making assumptions about templated types

The fillRand() function appears to assume that the underlying type is numeric, but there is no guarentee of that. We could write this:

MyMatrix<std::string> sm{3, 2};
sm.fillRand();

But it's unlikely to provide a satisfactory result because what happens is that it creates six strings, each one character long with the numeric value of the generated random value. For that reason, I'd suggest simply omitting that function. If you wish to only accomodate numeric values, then the code could include std::enable_if with the is_arithmetic type trait.

Consider implementing initializer list constructors

It would be nice to be able to do this:

MyMatrix<std::string> sm{3, 2, { "one", "two", "three", "four", "five", "six" }};

It's quite simple to accomodate this:

MyMatrix(size_type dimx, size_type dimy, std::initializer_list<T> init)
        : m_rows(dimx)
        , m_cols(dimy)
        , m_buffer(dimx * dimy)
{
    const size_type minlen{std::min(m_buffer.size(), init.size())};
    std::copy_n(init.begin(), minlen, m_buffer.begin());
}

Don't overspecify functions

The fill function should not be a member function since a user of the class can just as easily use the existing std::fill. I would same suggestion about the inserter function (std::ostream& operator<<). It's OK to have one as a convenience function for testing (which I would suggest would also need a wstream version), but I would recommend against having it in a library.

Write test cases

I would strongly recommend writing a large number of test cases to make sure this code does what you intend. It's easy to miss small details. Here's your first test case:

    MyMatrix<bool> b{5, 5};
    std::cout << b << "\n";

On my machine, this segfaults and dies. See if you can figure out why and fix it.

While it's great that the review enabled you to improve your code, please do not update the code in your question to incorporate feedback from answers. Doing so goes against the Question + Answer style of Code Review, as it unfortunately invalidates the existing review(s). This is not a forum where you should keep the most updated version in your question. Please see see what you may and may not do after receiving answers for ways to announce your new code. — Edward
– Edward, Commented Aug 14, 2020 at 20:25
The test case compiles for me with a warning. But did you mean that a const bool being passed by reference? Should it be passed by value? ( I use visual studio 2019's version of clang.) — Deviner
– Deviner, Commented Aug 17, 2020 at 23:15

Stack Exchange Network

Matrix template with a vector buffer

mymatrix.h

1 Answer 1

Use include guards

Use `throw` rather than `assert`

Implement mathematical operations with templates

Implement mathematical operations as freestanding functions

Don't shadow parameters

Implement unary operators

Fix the spelling errors

Implement operators with constant arguments

Eliminate work

Implement a `size()` operator

Avoid making assumptions about templated types

Consider implementing initializer list constructors

Don't overspecify functions

Write test cases

You must log in to answer this question.

Hot Network Questions

Matrix template with a vector buffer

mymatrix.h

1 Answer 1

Use include guards

Use throw rather than assert

Implement mathematical operations with templates

Implement mathematical operations as freestanding functions

Don't shadow parameters

Implement unary operators

Fix the spelling errors

Implement operators with constant arguments

Eliminate work

Implement a size() operator

Avoid making assumptions about templated types

Consider implementing initializer list constructors

Don't overspecify functions

Write test cases

You must log in to answer this question.

Related

Hot Network Questions

Use `throw` rather than `assert`

Implement a `size()` operator