patterncppMinor
Templated Matrix class
Viewed 0 times
templatedmatrixclass
Problem
I created a Matrix class. I tested it and it seems to work.
```
#ifndef MATRIX_H
#define MATRIX_H
#include
#include
#include
#include
#include
template
class Matrix
{
static_assert(std::is_arithmetic::value,"");
public:
Matrix(size_t, size_t);
Matrix(size_t, size_t, const T&);
Matrix(const Matrix&);
virtual ~Matrix();
void set(const T&);
size_t get_row() const;
size_t get_col() const;
void print(std::ostream&) const;
Matrix& operator=(const Matrix&);
T& operator()(size_t, size_t);
T operator()(size_t, size_t) const;
bool operator==(const Matrix&) const;
bool operator!=(const Matrix&) const;
Matrix& operator+=(const Matrix&);
Matrix& operator-=(const Matrix&);
Matrix operator+(const Matrix&) const;
Matrix operator-(const Matrix&) const;
Matrix& operator*=(const T& v);
Matrix& operator*=(const Matrix&);
Matrix operator*(const Matrix&) const;
private:
size_t row;
size_t col;
size_t dim;
T* data;
void copy(const Matrix&);
};
template
inline Matrix::Matrix(size_t r, size_t c)
: row(r), col(c), dim(rc), data(new T[rc])
{}
template
inline Matrix::Matrix(size_t r, size_t c, const T& v)
: row(r), col(c), dim(rc), data(new T[rc])
{
set(v);
}
template
inline Matrix::Matrix(const Matrix& m)
{
copy(m);
}
template
inline Matrix::~Matrix()
{
delete [] data;
}
template
inline void Matrix::set(const T& v)
{
for(size_t i(0); i
inline size_t Matrix::get_row() const
{
return row;
}
template
inline size_t Matrix::get_col() const
{
return col;
}
template
inline void Matrix::print(std::ostream& out) const
{
for(size_t i(0); i
inline Matrix& Matrix::operator=(const Matrix& m)
{
if(this != &m)
{
delete [] data;
copy(m);
}
- Can someone tell me if this class is good?
- Can I improve it?
- Can I use more move semantics (where)? What do I have to modify?
- Are there some logic/programming errors?
```
#ifndef MATRIX_H
#define MATRIX_H
#include
#include
#include
#include
#include
template
class Matrix
{
static_assert(std::is_arithmetic::value,"");
public:
Matrix(size_t, size_t);
Matrix(size_t, size_t, const T&);
Matrix(const Matrix&);
virtual ~Matrix();
void set(const T&);
size_t get_row() const;
size_t get_col() const;
void print(std::ostream&) const;
Matrix& operator=(const Matrix&);
T& operator()(size_t, size_t);
T operator()(size_t, size_t) const;
bool operator==(const Matrix&) const;
bool operator!=(const Matrix&) const;
Matrix& operator+=(const Matrix&);
Matrix& operator-=(const Matrix&);
Matrix operator+(const Matrix&) const;
Matrix operator-(const Matrix&) const;
Matrix& operator*=(const T& v);
Matrix& operator*=(const Matrix&);
Matrix operator*(const Matrix&) const;
private:
size_t row;
size_t col;
size_t dim;
T* data;
void copy(const Matrix&);
};
template
inline Matrix::Matrix(size_t r, size_t c)
: row(r), col(c), dim(rc), data(new T[rc])
{}
template
inline Matrix::Matrix(size_t r, size_t c, const T& v)
: row(r), col(c), dim(rc), data(new T[rc])
{
set(v);
}
template
inline Matrix::Matrix(const Matrix& m)
{
copy(m);
}
template
inline Matrix::~Matrix()
{
delete [] data;
}
template
inline void Matrix::set(const T& v)
{
for(size_t i(0); i
inline size_t Matrix::get_row() const
{
return row;
}
template
inline size_t Matrix::get_col() const
{
return col;
}
template
inline void Matrix::print(std::ostream& out) const
{
for(size_t i(0); i
inline Matrix& Matrix::operator=(const Matrix& m)
{
if(this != &m)
{
delete [] data;
copy(m);
}
Solution
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
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 ` 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.Context
StackExchange Code Review Q#14784, answer score: 5
Revisions (0)
No revisions yet.