Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I created a Matrix class. I tested it and it seems to work.

  1. Can someone tell me if this class is good?
  2. Can I improve it?
  3. Can I use more move semantics (where)? What do I have to modify?
  4. Are there some logic/programming errors?

#ifndef MATRIX_H
#define MATRIX_H

#include <iostream>
#include <initializer_list>
#include <stdexcept>
#include <utility>
#include <type_traits>

template <typename T>
class Matrix
{
    static_assert(std::is_arithmetic<T>::value,"");

public:
    Matrix(size_t, size_t);
    Matrix(size_t, size_t, const T&);
    Matrix(const Matrix<T>&);
    virtual ~Matrix();

    void set(const T&);
    size_t get_row() const;
    size_t get_col() const;

    void print(std::ostream&) const;

    Matrix<T>& operator=(const Matrix<T>&);
    T& operator()(size_t, size_t);
    T operator()(size_t, size_t) const;
    bool operator==(const Matrix<T>&) const;
    bool operator!=(const Matrix<T>&) const;
    Matrix<T>& operator+=(const Matrix<T>&);
    Matrix<T>& operator-=(const Matrix<T>&);
    Matrix<T> operator+(const Matrix<T>&) const;
    Matrix<T> operator-(const Matrix<T>&) const;
    Matrix<T>& operator*=(const T& v);
    Matrix<T>& operator*=(const Matrix<T>&);
    Matrix<T> operator*(const Matrix<T>&) const;

private:
    size_t row;
    size_t col;
    size_t dim;
    T* data;

    void copy(const Matrix<T>&);
};

template <typename T>
inline Matrix<T>::Matrix(size_t r, size_t c)
: row(r), col(c), dim(r*c), data(new T[r*c])
{}

template <typename T>
inline Matrix<T>::Matrix(size_t r, size_t c, const T& v)
: row(r), col(c), dim(r*c), data(new T[r*c])
{
    set(v);
}

template <typename T>
inline Matrix<T>::Matrix(const Matrix<T>& m)
{
    copy(m);
}

template <typename T>
inline Matrix<T>::~Matrix()
{
    delete [] data;
}

template <typename T>
inline void Matrix<T>::set(const T& v)
{
    for(size_t i(0); i < dim; i++)
    {
        data[i] = v;
    }
}

template <typename T>
inline size_t Matrix<T>::get_row() const
{
    return row;
}

template <typename T>
inline size_t Matrix<T>::get_col() const
{
    return col;
}

template <typename T>
inline void Matrix<T>::print(std::ostream& out) const
{
    for(size_t i(0); i < row; i++)
    {
        for(size_t j(0); j < col; j++)
        {
            out << data[col*i + j] << ' ';
        }

        out << std::endl;
    }
}

template <typename T>
inline Matrix<T>& Matrix<T>::operator=(const Matrix<T>& m)
{
    if(this != &m)
    {
        delete [] data;
        copy(m);
    }

    return *this;
}

template <typename T>
inline T& Matrix<T>::operator()(size_t i, size_t j)
{
    if( (i == 0) or (j == 0) or (i > row) or (j > col))
    {
        throw std::out_of_range("");
    }

    return data[col*(i-1) + (j-1)];
}

template <typename T>
inline T Matrix<T>::operator()(size_t i, size_t j) const
{
    if( (i == 0) or (j == 0) or (i > row) or (j > col))
    {
        throw std::out_of_range("");
    }

    return data[col*(i-1) + (j-1)];
}

template <typename T>
inline bool Matrix<T>::operator==(const Matrix<T>& m) const
{
    if( (row != m.row) or (col != m.col) )
    {
        return false;
    }

    for(size_t i(0); i < row; i++)
    {
        if(data[i] != m.data[i])
        {
            return false;
        }
    }

    return true;
}

template <typename T>
inline bool Matrix<T>::operator!=(const Matrix<T>& m) const
{
    return !((*this) == m);
}

template <typename T>
inline Matrix<T>& Matrix<T>::operator+=(const Matrix<T>& m)
{
    if( (row != m.row) or (col != m.col) )
    {
        throw std::range_error("");
    }

    for(size_t i(0); i < dim; i++)
    {
        data[i] += m.data[i];
    }

    return *this;
}

template <typename T>
inline Matrix<T>& Matrix<T>::operator-=(const Matrix<T>& m)
{
    if( (row != m.row) or (col != m.col) )
    {
        throw std::range_error("");
    }

    for(size_t i(0); i < dim; i++)
    {
        data[i] -= m.data[i];
    }

    return *this;
}

template <typename T>
inline Matrix<T> Matrix<T>::operator+(const Matrix<T>& m) const
{
    if( (row != m.row) or (col != m.col) )
    {
        throw std::range_error("");
    }

    Matrix<T> tmp(*this);

    for(size_t i(0); i < dim; i++)
    {
        tmp.data[i] += m.data[i];
    }

    return std::move(tmp);
}

template <typename T>
inline Matrix<T> Matrix<T>::operator-(const Matrix<T>& m) const
{
    if( (row != m.row) or (col != m.col) )
    {
        throw std::range_error("");
    }

    Matrix<T> tmp(*this);

    for(size_t i(0); i < dim; i++)
    {
        tmp.data[i] -= m.data[i];
    }

    return std::move(tmp);
}

template <typename T>
inline Matrix<T>& Matrix<T>::operator*=(const T& v)
{
    for(size_t i(0); i < dim; i++)
    {
        data[i] *= v;
    }

    return *this;
}

template <typename T>
inline Matrix<T>& Matrix<T>::operator*=(const Matrix<T>& m)
{
    if( col != m.row )
    {
        throw std::range_error("");
    }

    Matrix<T> tmp(*this);

    col = m.col;
    delete [] data;
    data = new T[row*col];

    for(size_t i(0); i < row; i++)
    {
        for(size_t j(0); j < m.col; j++)
        {
            for(size_t k(0); k < tmp.col; k++ )
            {
                data[col*i + j] += tmp.data[tmp.col*i + k] * m.data[m.col*k + j];
            }
        }
    }

    return *this;
}

template <typename T>
inline Matrix<T> Matrix<T>::operator*(const Matrix<T>& m) const
{
    if( col != m.row )
    {
        throw std::range_error("");
    }

    Matrix<T> tmp(*this);

    return std::move(tmp *= m);
}

template <typename T>
inline void Matrix<T>::copy(const Matrix<T>& m)
{
    row = m.row;
    col = m.col;
    dim = m.dim;

    data = new T[dim];

    for(size_t i(0); i < dim; i++)
    {
        data[i] = m.data[i];
    }
}

template <typename T>
inline Matrix<T> operator*(const T& v, const Matrix<T>& m)
{
    Matrix<T> tmp(m);

    tmp *= v;

    return std::move(tmp);
}

template <typename T>
inline Matrix<T> operator*(const Matrix<T>& m, const T& v)
{
    return std::move(v * m);
}

template <typename T>
inline std::ostream& operator<<(std::ostream& out, const Matrix<T>& m)
{
    m.print(out);

    return out;
}

#endif
share|improve this question

migrated from stackoverflow.com Aug 17 '12 at 15:04

This question came from our site for professional and enthusiast programmers.

    
i would rename get_row() to get_row_size(). I would expect a method called get_row to take an index and return a row vector if this index is between 0 and the row size –  stefan Aug 17 '12 at 15:15

2 Answers 2

up vote 6 down vote accepted

I would make several improvements.

I really don't like the fact that the interface has no names on parameters. You can skip all the names you want on the definitions, but when I look at a declaration I want to know what the parameters are.

Still on the interface part, I'd rename get_row and get_col to rows and columns: they don't get you a row or a column, but the total number of rows or columns. set I would probably name fill, but then I'd remember there's a std::fill algorithm already in the <algorithm> header. That would lead me to think about providing an iterator-based interface: it brings the ability to use all the standard algorithms. I'll come back to this point later.

Then, I see no reason to provide a virtual destructor. Why would Matrix be a polymorphic base class? If you use it polymorphically you lose the ability to properly pass it by value, and that makes the overloaded operators add confusion and opportunities for slicing.

Unless you are using the Visual Studio compiler (which has this known issue), there is no need to move local variables in return statements like return std::move(tmp);, or even to move temporaries like return v * m;. The compiler does that automatically. Making the move explicit causes confusion, at least to those that are already acquainted with the C++11 features.

The biggest change I'd make would be to follow what I call the rule of zero. I'd use an existing solution for handling memory ownership. In this case, a std::vector member. That means I don't need to write a copy constructor, nor a copy assignment operator, nor a destructor: I get all those for free and a move constructor and a move assignment operator. Less code and more features.

And now that I have a std::vector member, I can use it to easily provide an iterator based interface: just return iterators from the vector, and everything works.

share|improve this answer
    
how about assign for set/fill? –  sehe Aug 17 '12 at 15:24
    
Thank you for the advices! I will change my T* pointer in a std::vector<T>. I don't get very well how to provide iterators (also because I didn't studied the topic very well). –  R. M. Aug 17 '12 at 15:40
    
@R.M. obviously feel free to disregard the parts about iterators right until you start understanding them. If you decide to get on with and have trouble I'll be happy to help you with it on Stack Overflow or giving advice here :) –  R. Martinho Fernandes Aug 17 '12 at 15:49
    
@R.MartinhoFernandes Fernandes Tomorrow I will post my code modification and maybe I can better study iterators. Thank you for the advices! ;) –  R. M. Aug 17 '12 at 16:00
    
I totally agree in the real world I would use std::vector<T> but this is a perfect learning exercise to show how to implement copy construction assignment and move semantics for a class you write. It is simple but complex enough to have a few got-ya's in it. –  Loki Astari Aug 17 '12 at 17:54

One general comment is that, instead of catching dimensional mis-matches in assignments and operations between matrices at runtime, you can add two more template parameters for the number of rows and columns respectively. Then you can catch these mis-matches at compile time. I have done this in two matrix implementations in the past and it works quite well.

template <typename T, unsigned int ROWS, unsigned int COLS>
class Matrix { .... };

Of course, this means that different size matrices are different types.

share|improve this answer
    
I thought this was a very good idea while I arrived at matrix multiplication. If I multiply a 3x2 matrix with a 2x7 matrix, I have to create a different 3x7 matrix to store the result... So I thought it was better to let matrices change dimension. –  R. M. Aug 17 '12 at 15:38
    
@R.M. When i designed my matrices, I thought it better to make sure at compile time that such a multiplication could only be assigned to a 3x7 matrix. I found too many bugs in code using dynamically sized matrices... –  juanchopanza Aug 17 '12 at 15:43
    
I will think about it, but I think is quite annoying calculating the dimension of the result. If I decide to use non-type templates, do you think that I have to store the number of rows and columns anyway? –  R. M. Aug 17 '12 at 15:49
    
@R.M. If you have C++11, then you can use auto for the return type. If not, it can be a pain but at least the compiler will tell you you got the wrong size. As for storing the dimensions, it depends. If you were to store the data as a vector of vectors, then you should use their size(). –  juanchopanza Aug 17 '12 at 15:54
    
I think I will stay to store data in a std::vector<T>: it's more efficient in allocation. –  R. M. Aug 17 '12 at 15:58

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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