8
\$\begingroup\$

I've made a simple Matrix class to learn C++. It doesn't use Templates so it only works with int. I made it this way to really understand how to work with objects and memory. I'll probably move to the next level once I get feedback on this one.

I used these two blog posts as reference:

Description

  • The numbers (int) are stored in a 1D array of fixed size. We can reshape the matrix but the new shape can't modify the total size
  • It supports basic matrix and scalar operations (+,-,*)
  • Some helpers functions are available like filling the matrix with one value, a range or random integers

One question I have is when I access a member of an instance, what is the difference between using this->m_member or just m_member (if there is one)?

Also, when an instance is passed by reference, I can access its members even the private ones. You can see this behavior on the implementation of the copy constructor and at many other places. Is it good practice? Or should I have a public getter for accessing the private member of a class, even when they are the same type?

CMatrix.hpp

#ifndef CMatrix_hpp
#define CMatrix_hpp


#include <iostream>
#include <random>

class CMatrix {
private:
    int* m_ptValues;
    int m_totalSize;
    int m_rows;
    int m_columns;
public:
    CMatrix(int rows, int columns); // base ctor.
    CMatrix(const CMatrix& rhs); // copy ctor.
    CMatrix& operator=(const CMatrix& rhs); // assign. ctor.
    ~CMatrix(); // dtor.
    int& operator()(int row, int column);
    int& operator()(int row, int column) const;
    CMatrix& operator+=(int scalar);
    CMatrix operator+(int scalar);
    CMatrix& operator-=(int scalar);
    CMatrix operator-(int scalar);
    CMatrix& operator*=(int scalar);
    CMatrix operator*(int scalar);
    CMatrix& operator*=(const CMatrix& rhs);
    CMatrix operator*(const CMatrix& rhs);
    CMatrix& operator+=(const CMatrix& rhs);
    CMatrix operator+(const CMatrix& rhs);
    void reshape(int newRows, int newColumns);
    void show(); //used for dev. only
    void range(int start, int defaultStep=1);
    void fill(int value);
    void randint(int lowerBound, int upperBound);
};

#endif /* CMatrix_hpp */

CMatrix.cpp

#include "CMatrix.hpp"

CMatrix::CMatrix(int rows, int columns) : m_rows(rows), m_columns(columns) {
    m_totalSize = m_rows * m_columns;
    m_ptValues = new int[m_totalSize]();
}
CMatrix::CMatrix(const CMatrix& rhs) : m_rows(rhs.m_rows), m_columns(rhs.m_columns) {
    m_totalSize = rhs.m_totalSize;
    m_ptValues = new int[rhs.m_totalSize]();
    std::memcpy(m_ptValues, rhs.m_ptValues, rhs.m_totalSize * sizeof(int));
}
CMatrix& CMatrix::operator=(const CMatrix& rhs) {
    if (&rhs == this) {
        return *this;
    }
    if (m_totalSize == rhs.m_totalSize) {
        std::memcpy(m_ptValues, rhs.m_ptValues, rhs.m_totalSize * sizeof(int));
    } else {
        delete[] m_ptValues;
        m_ptValues = new int[rhs.m_totalSize]();
        std::memcpy(m_ptValues, rhs.m_ptValues, rhs.m_totalSize * sizeof(int));
    }
    m_rows = rhs.m_rows;
    m_columns = rhs.m_columns;
    m_totalSize = rhs.m_totalSize;

    return *this;
}
CMatrix::~CMatrix() {
    if (m_ptValues) {
        delete[] m_ptValues;
    }
}
int& CMatrix::operator()(int row, int column) {
    if (row >= m_rows || column >= m_columns) {
        throw std::out_of_range("Index out of bounds");
    }
    return m_ptValues[row * m_columns + column];
}
int& CMatrix::operator()(int row, int column) const {
    if (row >= m_rows || column >= m_columns) {
        throw std::out_of_range("Index out of bounds");
    }
    return m_ptValues[row * m_columns + column];
}
CMatrix& CMatrix::operator+=(int scalar) {
    for (auto i = 0; i < m_totalSize; i++) {
        m_ptValues[i] += scalar;
    }
    return *this;
}
CMatrix CMatrix::operator+(int scalar) {
    CMatrix result(m_rows, m_columns);

    for (auto i = 0; i < m_totalSize; i++) {
        result.m_ptValues[i] = m_ptValues[i] + scalar;
    }
    return result;
}
CMatrix& CMatrix::operator-=(int scalar) {
    for (auto i = 0; i < m_totalSize; i++) {
        m_ptValues[i] -= scalar;
    }
    return *this;
}
CMatrix CMatrix::operator-(int scalar) {
    CMatrix result(m_rows, m_columns);

    for (auto i = 0; i < m_totalSize; i++) {
        result.m_ptValues[i] = m_ptValues[i] - scalar;
    }
    return result;
}
CMatrix& CMatrix::operator*=(int scalar) {
    for (auto i = 0; i < m_totalSize; i++) {
        m_ptValues[i] *= scalar;
    }
    return *this;
}
CMatrix CMatrix::operator*(int scalar) {
    CMatrix result(m_rows, m_columns);

    for (auto i = 0; i < m_totalSize; i++) {
        result.m_ptValues[i] = m_ptValues[i] * scalar;
    }
    return result;
}
CMatrix& CMatrix::operator*=(const CMatrix& rhs) {
    CMatrix result = (*this) * rhs;
    (*this) = result;
    return *this;
}
CMatrix CMatrix::operator*(const CMatrix& rhs) {
    if (m_columns != rhs.m_rows) {
        throw std::length_error("Matrices shapes mismatch");
    }
    CMatrix result(m_rows, rhs.m_columns);
    int sum;
    for (auto i = 0; i < m_rows; i++) {
        for (auto j = 0; j < rhs.m_columns; j++) {
            sum = 0;
            for (auto k = 0; k < m_rows; k++) {
                sum += this->operator()(i, k) * rhs(k,j);
            }
            result(i,j) = sum;
        }
    }
    return result;
}
CMatrix& CMatrix::operator+=(const CMatrix& rhs) {
    CMatrix result = (*this) + rhs;
    (*this) = result;
    return *this;
}
CMatrix CMatrix::operator+(const CMatrix& rhs) {
    if (m_rows != rhs.m_rows || m_columns != rhs.m_columns) {
        throw std::length_error("Matrices shapes mismatch");
    }
    CMatrix result(m_rows, m_columns);
    for (auto i = 0; i < m_totalSize; i++) {
        result.m_ptValues[i] = this->m_ptValues[i] + rhs.m_ptValues[i];
    }
    return result;
}
void CMatrix::reshape(int newRows, int newColumns) {
    if (newRows * newColumns > m_totalSize) {
        throw std::invalid_argument("Total size of new matrix must be unchanged");
    }
    m_rows = newRows;
    m_columns = newColumns;
}
void CMatrix::show() {
    std::string delimiter = "";
    for (auto i = 0; i < m_rows; i++) {
        delimiter = "";
        for (auto j = 0; j < m_columns; j++) {
            std::cout << delimiter << m_ptValues[i * m_columns + j];
            delimiter = ",";
        }
        std::cout << std::endl;
    }
}
void CMatrix::range(int start, int defaultStep) {
    int counter = start;
    for (auto i = 0; i < m_totalSize; i++) {
        m_ptValues[i] = counter;
        counter += defaultStep;
    }
}
void CMatrix::fill(int value) {
    for (auto i = 0; i < m_totalSize; i++) {
        m_ptValues[i] = value;
    }
}
void CMatrix::randint(int lowerBound, int upperBound) {
    std::random_device rd;
    std::mt19937 mt;
    std::uniform_int_distribution<int> dist(lowerBound, std::nextafter(upperBound, __DBL_MAX__));
    for (auto i = 0; i < m_totalSize; i++) {
        m_ptValues[i] = dist(mt);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Your implementation does not use templates, but you might want to read more about STL. They are 1000X more convenient than pointers with new/delete, and very little overhead. They do hide away some of the memory management that you seem to want to delve into a little. \$\endgroup\$ – Aaron D. Marasco Feb 20 '17 at 13:29
  • \$\begingroup\$ Thanks, I will look into std::array and std::vector as the answer suggested. Would you say it is more common to do memory management or use a library that does it for us (like STL) in modern C++ ? \$\endgroup\$ – Anjou Feb 20 '17 at 18:09
  • \$\begingroup\$ I'm of the opinion that I'm a pretty smart guy, but there's always somebody smarter. For example, many more eyes than my own have examined Boost (uBLAS). I'm a fan of "standing on the shoulders of giants." \$\endgroup\$ – Aaron D. Marasco Feb 21 '17 at 0:07
6
\$\begingroup\$

Looking at your header first:

#include <iostream>
#include <random>

Your interface doesn't use any of the names defined in these headers, so no need to include them here. (You may find you need them in the implementation, but they are not required for the header).


int& operator()(int row, int column);
int& operator()(int row, int column) const;

The first of those looks good, but there's a mistake in your second one. If the object is const, then you shouldn't allow the calling code to change its elements by returning modifiable references to them. Either of the following is consistent:

const int& operator()(int row, int column) const;
int operator()(int row, int column) const;

Next, these methods:

void reshape(int newRows, int newColumns);
void show(); //used for dev. only
void range(int start, int defaultStep=1);
void fill(int value);
void randint(int lowerBound, int upperBound);

I'm not sure any of these methods belong in the public interface. If you provide suitable iterator types, you may be able to outsource some or all of these to standard algorithm functions.


Moving on to the implementation file, I'm a little concerned that you're doing your own memory allocation and deallocation. You could use a std::vector<int> for your values, and you'd get your memory management and copy constructor for free. I'd suggest members like

class CMatrix {
private:
    std::vector<int> values;
    // size is found by values.size()
    int rows;
    // columns is values.size()/rows
}

This gives you a more convenient form of your loops:

for (auto& i: values)
    ...;

Range checking:

int& CMatrix::operator()(int row, int column) {
    if (row >= m_rows || column >= m_columns) {
        throw std::out_of_range("Index out of bounds");
    }

If you're going to do range checking, you should also check that 0 <= row and 0 <= column - or guarantee it, by changing the interface to take an unsigned type (e.g. size_t).


CMatrix CMatrix::operator+(int scalar) {
    CMatrix result(m_rows, m_columns);

    for (auto i = 0; i < m_totalSize; i++) {
        result.m_ptValues[i] = m_ptValues[i] + scalar;
    }
    return result;
}

The usual idiom for operator+() is to implement it in terms of operator+=():

CMatrix CMatrix::operator+(int scalar) const {
    auto result = *this;
    return result += scalar;
}

Note that the this object is const here: we're only modifying a copy.

The other operators can follow a similar pattern. In particular, you have * and *= the other way around.


if (newRows * newColumns > m_totalSize) {
    throw std::invalid_argument("Total size of new matrix must be unchanged");
}

Your error message doesn't match what you're testing - must it be unchanged, or just not bigger?


void CMatrix::show() {
    std::string delimiter = "";
    for (auto i = 0; i < m_rows; i++) {
        delimiter = "";
        for (auto j = 0; j < m_columns; j++) {
            std::cout << delimiter << m_ptValues[i * m_columns + j];
            delimiter = ",";
        }
        std::cout << std::endl;
    }
}

I think you may benefit from passing the output stream as a parameter here, so you can (e.g.) print to cerr or to a file. It probably makes sense to implement operator<<() in the conventional manner instead of (or as well as) this member function. Either way, the matrix should be const.


To answer the quick questions:

  • Yes, in a member function, m_member is the same as this->m_member unless you declare a local of the same name.
  • You can (and should, where necessary) access the private members of all objects of the same type - the access modifiers are a property of the code, not the particular instance you're manipulating.
\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. Regarding the vector, can I use the std::array instead? From my understanding, a vector takes more memory to be able to expand because it's a dynamic container. Since I don't allow the number of elements to change, I don't think it is a good idea to use a dynamic container but I totally understand your point about memory management. Are you concerned because it is wrong/I'm a beginner or because it is less common to do memory management in C++ 11/14 ? \$\endgroup\$ – Anjou Feb 20 '17 at 18:00
  • 1
    \$\begingroup\$ For std::array, you need to know the number of elements at compilation time. That would make the size part of your type (probably as a template parameter). For std::vector, you don't need it until runtime, and it's then more likely a property of each object. You'll probably find the vector easiest to use here, since you do need the number of elements to change - in the assignment operator, most obviously. \$\endgroup\$ – Toby Speight Feb 20 '17 at 18:55
2
\$\begingroup\$

An important point here (a.k.a. a bug), the number of rows and columns is changed by this function:

void CMatrix::reshape(int newRows, int newColumns) {
    if (newRows * newColumns > m_totalSize) {
        throw std::invalid_argument("Total size of new matrix must be unchanged");
    }
    m_rows = newRows;
    m_columns = newColumns;
}

The function needs to also update the m_totalSize parameter:

m_totalSize = m_rows * m_columns;

I personally would not have such a function. I don't think that it makes sense to change a matrix width or height just like that.


In the assignment operator, you delete and reallocate the buffer if not exactly equal, which is contradictory compared that the reshape() above.

The if(m_totalSize == rhs.m_totalSize) could be if(m_totalSize >= rhs.m_totalSize). You lose some bytes in the process but avoid a delete + new which are generally slow.

CMatrix& CMatrix::operator=(const CMatrix& rhs) {
    if (&rhs == this) {
        return *this;
    }
    if (m_totalSize == rhs.m_totalSize) {  // replace '==' with '<='
        std::memcpy(m_ptValues, rhs.m_ptValues, rhs.m_totalSize * sizeof(int));
    } else {
        delete[] m_ptValues;
        m_ptValues = new int[rhs.m_totalSize]();
         std::memcpy(m_ptValues, rhs.m_ptValues, rhs.m_totalSize * sizeof(int));
    }
    m_rows = rhs.m_rows;
    m_columns = rhs.m_columns;
    m_totalSize = rhs.m_totalSize;

    return *this;
}

Another thing, with c++11 and newer, you should use nullptr when you test a pointer, as in:

CMatrix::~CMatrix() {
    if (m_ptValues != nullptr) {
        delete[] m_ptValues;
    }
}

Also delete is smart enough to test for null pointers, so really you could simplify with:

CMatrix::~CMatrix() {
    delete[] m_ptValues;
}

(that being said, there are compilers that can be setup to remove that test, so it can be useful to keep it around. Some people working on embedded systems view it as an optimization.

Also in your case, that pointer can't be null. If a constructor can't allocate that memory buffer, it will throw an std::bad_alloc error.

Finally, you should use an std::unique_ptr<>() instead (watch out for the copy operator when using that one!) That way it always automatically gets deleted or as mentioned in the other answer, use std::vector<>. Most STL implementation tell you if your index is out of bounds when compiled in debug mode or you use the at() member.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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