C++ Matrix Class - Suggestions

I am trying to build a templated Matrix class in C++. Below is its implementation. I implemented two operators +,+= as of now just to give an idea on how it will look and I thought it would be best to ask for feedback before I proceed any further.

The whole implementation is public and also there are no explicit bound/error checks, this is because its not meant to be a full fledged matrix library and hence avoided unnecessary code.

It would be very helpful if someone could开发者_StackOverflow社区 comment on it and may be suggest a few improvements or suggestions.

Thank you.

template<class T>
class Matrix
{
    public:
    int r,c;
    vector< vector< T > >mat;

    Matrix() {}

    // Constructor to set the size of the matrix
    Matrix(int _r,int _c)
    {
        r=_r;c=_c;
        mat.resize(r);
        for(int i=0;i<r;i++)
            mat[i].resize(c);
    }
    // Constructor to build a matrix from a C 2d array
    // Pointer to the first element is passed (&arr[0][0])
    Matrix(T *arr,int _r,int _c)
    {
        r=_r;c=_c;
        mat.resize(r);
        for(int i=0;i<r;i++)
            for(int j=0;j<c;j++)
                mat[i].push_back(arr[i*c+j]);
    }
    template<typename U>
    Matrix<T>& operator +=(const Matrix<U>&M)
    {
        for(int i=0;i<r;i++)
            for(int j=0;j<c;j++)
                mat[i][j]+=static_cast<T>(M.mat[i][j]);
        return *this;
    }
    template<typename U>
    Matrix<T> operator +(const Matrix<U>&M)
    {
        Matrix<T>tmp=*this;
        return tmp+=M;
    }
};

template<typename T>
istream& operator >>(istream &in,Matrix<T>&M)
{
    in>>M.r>>M.c;
    Matrix<T>tmp(M.r,M.c);
    for(int i=0;i<M.r;i++)
        for(int j=0;j<M.c;j++)
            in>>tmp.mat[i][j];
    M=tmp;
    return in;
}
template<typename T>
ostream& operator <<(ostream &out,Matrix<T>M)
{
    for(int i=0;i<M.r;i++)
    {
        for(int j=0;j<M.c;j++)
            cout<<M.mat[i][j]<<" ";
        cout<<endl;
    }
    return out;
}

EDIT: Thank you all for the suggestions.

I just have one small question, say I do want to implement error checking (ex: checking for bounds,valid arguments etc..) however I do want the provide user with an option to disable error checking completely, is there any good way to implement this ? What I need is something like example:`ios_base::sync_with_stdio(0);. Thanks again.

---

A couple of points:

• Use a single std::vector<T> instead of std::vector<std::vector<T>>. Index it with y*r+x - use and operator overload to make this easier (see next point). This will be more memory efficient and slightly faster (and your init will be a lot easier: resize(r*c)).
• Make your data members private to protect your matrix against unintentional size changes. For example, user code can currently resize the vectors but leave the old r and c.
• Overload operator() to access the matrix (both const and non-const). If you really must use the matrix[r][c] syntax instead of matrix(r,c), consider overloading operator[] and returning an iterator to the correct row (vector iterators are random access, so they will provide operator[]).
• Implement operator+ as a non-friend non-member function instead - Improves encapsulation!
• Use initialization lists as others suggested.
• Let the constructor that currently takes a T* take an iterator instead. That way you automagically get the pointers support along with a lot of other cool things, such as range checking for debug-iterators, automatic type conversion for compatible value types and support for all other random access iterators. Also consider filling your matrix row-wise, so you can use forward iterators too.

---

If you want to know how it's done properly, see the Eigen project's implementation:

https://bitbucket.org/eigen/eigen/src/49e00a2e570c/Eigen/src/Core/Matrix.h

https://bitbucket.org/eigen/eigen/src/49e00a2e570c/Eigen/src/Core/MatrixBase.h

Eigen is a template-heavy fast matrix library for C++. I'm sure there's a lot of usefulness there.

---

1 .Use initializer lists for constructor like, Matrix (...) : r(_r), c(_c)

2.operator + can be simply { return this->Add(M) } (assuming that typenames T and U are compatible and you have implemented some Add() method). 3. If you are constructing something from 2D array then use below technique:

template<typename T>
class Matrix {
public: // ...
  void Construct (T *arr)
  {
    mat.resize(r);
    for(int i=0;i<r;i++)
      for(int j=0;j<c;j++)
        mat[i].push_back(arr[i*c+j]);
  }
  template<size_t ROW, size_t COL>
  Matrix(T (&arr)[ROW][COL]) : r(ROW), c(COL)
  {
    Construct(arr);
  }
//...
};

Thus you don't have to pass the size of the 2D array when you are calling and it will reduce the probability of errors.

1. For better readability, you can use better names for class members: either this->r or t_r.

---

Use initialization lists when initializing contructor args:

 Matrix(int _r,int _c)
    {
        r=_r;c=_c;

..

should be:

 Matrix(int _r,int _c) : r(_r), c(_c)
    {
        ....

---

1. You might want to make r, c and mat members private.
2. Your streaming operators are not symmetric (operator<< doesn't write out r and c).

---

I wouldn't use a vector of vectors to achieve this. This way you're adding quite some overhead to all lookups without any real need for it. Just allocate a one or two dimensional array of T and use that one within your class instead. This would also allow fast copying to other object instances without having to iterate through all fields as well as resetting the whole matrix.

Also you should make your members const when they shouldn't be changed later on (in your example r and c). To initialize them just use the constructor list in Tony's post.

---

You're wasting your time keeping _r and _c because the vectors keep their own sizes.

---

I wonder why you used the very expensive push_back function in this Class initialization:

// Pointer to the first element is passed (&arr[0][0])
Matrix(T *arr,int _r,int _c)
{
    r=_r;c=_c;
    mat.resize(r);
    for(int i=0;i<r;i++)
        for(int j=0;j<c;j++)
            mat[i].push_back(arr[i*c+j]);
}

Since you already have the number of rows and columns, consider this:

// Pointer to the first element is passed (&arr[0][0])
Matrix(T *arr,int _r,int _c)
{
    r=_r;c=_c;
    mat.resize(r);
    for(int i=0;i<r;i++)
        mat[i].resize(c);
        for(int j=0;j<c;j++)
            mat[i][j]= arr[i*c+j];
}

---

If speed is what you have in mind, don't implement a matrix library yourself but use Boost libraries, I've used them in some projects and do pretty well on both 32 and 64 bit architectures