4
\$\begingroup\$

I need to write a CustomBasicMatrix class implementation for college. This includes basic operations such as int and CustomBasicMatrix addition and subtraction, Transposition and iterations / const iterations.

I would like to know if there are coding conventions, or more importantly, Memory Leak / Segmentation Faults Possibilities.

Context:

  • Implementation must be wrapped in a namespace "sys".
  • The only external file in use is "Auxiliaries.h", which contains: "Dimension" struct, only holds 'width' and 'height', with additional helper functions for printing a matrix.
  • Instead of transposing a function by flipping dimensions, I saved a boolean value indicates if the matrix is transposed, thus when fetching the value we use said boolean to determine if we fetch (i,j) or (j,i). This is to save memory and overhead.

Thanks in advance!

CustomBasicMatrix .h:


#include <ostream>
#include "Auxiliaries.h"

namespace sys{
    class CustomBasicMatrix{
    private:
        int rows;
        int cols;
        int** data;
        bool trans = false;
    public:
        explicit CustomBasicMatrix(Dimensions dim, int initValue = 0);
        CustomBasicMatrix(const CustomBasicMatrix& other);
        virtual ~CustomBasicMatrix();
        CustomBasicMatrix& operator=(const CustomBasicMatrix& other);


        static CustomBasicMatrix Identity(int dims);

        int height() const;
        int width() const;
        int size() const;

        CustomBasicMatrix transpose() const;



        CustomBasicMatrix operator-() const;
        CustomBasicMatrix& operator+=(int scalar);
        CustomBasicMatrix& operator+=(const CustomBasicMatrix& rhs);

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

        class iterator{
        private:
            CustomBasicMatrix* matrix;
            int row;
            int col;
        public:
            iterator(CustomBasicMatrix* matrix, int row=0, int col=0);
            virtual ~iterator() = default;
            iterator& operator=(const iterator& other);

            int& operator*();
            iterator& operator++();
            const iterator operator++(int);

            bool operator==(const iterator& other) const;
            bool operator!=(const iterator& other) const;
        };

        iterator begin();
        iterator end();


        class const_iterator{
        private:
            const CustomBasicMatrix* matrix;
            int row;
            int col;
        public:
            const_iterator(const CustomBasicMatrix *matrix, int row = 0, int col = 0);
            virtual ~const_iterator() = default;
            const_iterator& operator=(const const_iterator& other);

            const int& operator*() const;
            const_iterator operator++();
            const const_iterator operator++(int);

            bool operator==(const const_iterator& other) const;
            bool operator!=(const const_iterator& other) const;

        };

        const_iterator begin() const;
        const_iterator end() const;
    };

    bool any(const CustomBasicMatrix& mat);
    bool all(const CustomBasicMatrix& mat);

    CustomBasicMatrix operator+(const CustomBasicMatrix& lhs, const CustomBasicMatrix& rhs);
    CustomBasicMatrix operator+(const CustomBasicMatrix& lhs, int scalar);
    CustomBasicMatrix operator+(int scalar, const CustomBasicMatrix& matrix);
    CustomBasicMatrix operator-(const CustomBasicMatrix &lhs, const CustomBasicMatrix &rhs);

    CustomBasicMatrix operator<(const CustomBasicMatrix& lhs, int scalar);
    CustomBasicMatrix operator<=(const CustomBasicMatrix& lhs, int scalar);
    CustomBasicMatrix operator>(const CustomBasicMatrix& lhs, int scalar);
    CustomBasicMatrix operator>=(const CustomBasicMatrix& lhs, int scalar);
    CustomBasicMatrix operator!=(const CustomBasicMatrix& lhs, int scalar);
    CustomBasicMatrix operator==(const CustomBasicMatrix& lhs, int scalar);
    std::ostream &operator<<(std::ostream &os, const CustomBasicMatrix &matrix);
}

CustomBasicMatrix .cpp:


#include "CustomBasicMatrix.h"
#include "Auxiliaries.h"

#define CBM sys::CustomBasicMatrix

CBM::CustomBasicMatrix(sys::Dimensions dim, int initValue)
{
    this->rows = dim.getRow();
    this->cols = dim.getCol();
    this->data = new int*[this->rows];
    int i;
    try{
        for (i = 0; i < this->rows; ++i){
            this->data[i] = new int[this->cols];
        }
    } catch(const std::exception& e){
        for (int j = 0; j < i; ++j){
            delete[] this->data[j];
        }
        delete[] this->data;
        throw e;
    }

    for(int i=0; i< this->rows ; i++){
        for (int j = 0; j < this->cols; ++j){
            this->data[i][j] = initValue;
        }

    }
}

CBM::CustomBasicMatrix(const CBM &other)
{
    this->rows = other.rows;
    this->cols = other.cols;
    this->trans = other.trans;

    this->data = new int*[this->rows];
    int i;
    try{
        for (i = 0; i < this->rows; ++i){
            this->data[i] = new int[this->cols];
        }
    } catch(const std::exception& e){
        for (int j = 0; j < i; ++j){
            delete[] this->data[j];
        }
        delete[] this->data;
        throw e;
    }

    for(int i=0; i< this->rows ; i++){
        for (int j = 0; j < this->cols; ++j){
            this->data[i][j] = other.data[i][j];
        }
    }
}

CBM::~CustomBasicMatrix()
{
    for (int i = 0; i < this->rows; ++i){
        delete[] this->data[i];
    }
    delete[] this->data;
}

CBM &CBM::operator=(const CBM &other)
{
    if(this == &other) return *this;

    for (int i = 0; i < this->rows; ++i){
        delete[] this->data[i];
    }
    delete[] this->data;

    this->rows = other.rows;
    this->cols = other.cols;
    this->trans = other.trans;

    this->data = new int*[this->rows];
    int i;
    try{
        for (i = 0; i < this->rows; ++i){
            this->data[i] = new int[this->cols];
        }
    } catch(const std::exception& e){
        for (int j = 0; j < i; ++j){
            delete[] this->data[j];
        }
        delete[] this->data;
        throw e;
    }

    for(int i=0; i< this->rows ; i++){
        for (int j = 0; j < this->cols; ++j){
            this->data[i][j] = other.data[i][j];
        }
    }

    return *this;
}

CBM CBM::Identity(int dims)
{
    Dimensions dim = Dimensions(dims, dims);
    CustomBasicMatrix ret(dim, 0);

    for (int i = 0; i < dims; ++i){
        ret.data[i][i] = 1;
    }

    return ret;
}

int CBM::height() const
{
    return this->trans ? this->cols : this->rows;
}

int CBM::width() const
{
    return this->trans ? this->rows : this->cols;
}

int CBM::size() const
{
    return this->rows * this->cols;
}

CBM CBM::transpose() const
{
    CustomBasicMatrix ret(*this);
    ret.trans = !ret.trans;
    return ret;
}

CBM& CBM::operator+=(int scalar)
{
    for (int i = 0; i < this->rows ; ++i){
        for (int j = 0; j < this->cols ; ++j){
            this->data[i][j] += scalar;
        }
    }
    return *this;
}

CBM &CBM::operator+=(const CBM &rhs)
{
    for (int i = 0; i < this->rows ; ++i){
        for (int j = 0; j < this->cols ; ++j){
            this->data[i][j] += rhs.data[i][j];
        }
    }
    return *this;
}

CBM CBM::operator-() const
{
    CustomBasicMatrix reg(*this);

    for (int i = 0; i < reg.rows ; ++i){
        for (int j = 0; j < reg.cols; ++j){
            reg.data[i][j] = -reg.data[i][j];
        }
    }

    return reg;
}

int &CBM::operator()(int row, int col)
{
    if(this->trans)
        return this->data[col][row];
    else
        return this->data[row][col];
}

int &CBM::operator()(int row, int col) const
{
    if(this->trans)
        return this->data[col][row];
    else
        return this->data[row][col];
}


CBM sys::operator+(const CBM &lhs, const CBM &rhs)
{
    CBM temp(lhs);
    return (temp += rhs);
}

CBM sys::operator+(const CBM &lhs, int scalar)
{
    CBM temp = lhs;
    return (temp += scalar);
}

CBM sys::operator-(const CBM &lhs, const CBM &rhs)
{
    CBM temp = lhs;
    return (temp += -rhs);
}


CBM sys::operator<(const CBM& lhs, int scalar)
{
    CBM res(lhs);
    for (int i = 0; i < res.height() ; ++i){
        for (int j = 0; j < res.width(); ++j){
            res(i,j) = res(i,j) < scalar;
        }
    }
    return res;
}

CBM sys::operator<=(const CBM& lhs, int scalar)
{
    CBM res1 = lhs == scalar;
    CBM res2 = lhs < scalar;
    return (res1 += res2);
}

CBM sys::operator>(const CBM& lhs, int scalar)
{
    CBM res(lhs);
    for (int i = 0; i < res.height() ; ++i){
        for (int j = 0; j < res.width(); ++j){
            res(i,j) = res(i,j) > scalar;
        }
    }

    return res;
}

CBM sys::operator>=(const CBM& lhs, int scalar)
{
    CBM res1 = lhs == scalar;
    CBM res2 = lhs > scalar;
    return res1 += res2;
}

CBM sys::operator!=(const CBM& lhs, int scalar)
{
    CBM res1 = lhs > scalar;
    CBM res2 = lhs < scalar;
    return res1 += res2;
}

CBM sys::operator==(const CBM& lhs, int scalar)
{
    CBM res(lhs);
    for (int i = 0; i < res.height() ; ++i){
        for (int j = 0; j < res.width(); ++j){
            res(i,j) = res(i,j) == scalar;
        }
    }   return res;
}

CBM sys::operator+(int scalar, const CBM &matrix)
{
    return matrix + scalar;
}

CBM::iterator CBM::begin()
{
    return iterator (this);
}

CBM::iterator CBM::end()
{
    return iterator(this, this->rows, this->cols);
}

bool sys::any(const CBM &mat)
{
    for (CBM::const_iterator it = mat.begin() ; it != mat.end() ; it++){
        if((*it) != 0) return true;
    }
    return false;
}

bool sys::all(const CBM &mat)
{
    for (CBM::const_iterator it = mat.begin() ; it != mat.end() ; it++){
        if((*it) == 0) return false;
    }
    return true;
}

CBM::const_iterator CBM::begin() const
{

    return const_iterator(this);
}

CBM::const_iterator CBM::end() const
{

    return const_iterator(this, this->rows, this->cols);
}

std::ostream &sys::operator<<(std::ostream &os, const CBM &matrix)
{
    int *vals = new int[matrix.size()];

    for (int i = 0; i < matrix.height(); ++i){
        for (int j = 0; j < matrix.width(); ++j){
            vals[i * matrix.width() + j] = matrix(i,j);
        }
    }

    Dimensions dim(matrix.height(), matrix.width());
    std::string res = printMatrix(vals, dim);
    delete[] vals;

    return os << res;
}

/************************************/


CBM::iterator::iterator(CBM *matrix, int row, int col) : matrix(matrix), row(row), col(col){}


CBM::iterator &CBM::iterator::operator=(const iterator& other)
{
    if(this == &other) return *this;

    this->matrix = other.matrix;
    this->row = other.row;
    this->col = other.col;
    return *this;
}

int &CBM::iterator::operator*()
{
    return this->matrix->operator()(this->row, this->col);
}

CBM::iterator &CBM::iterator::operator++()
{
        this->col++;
        this->row += this->col / this->matrix->cols;
        this->col = this->col % this->matrix->cols;

    if(this->row == this->matrix->rows || this->col == this->matrix->cols){
        this->row = this->matrix->rows;
        this->col = this->matrix->cols;
    }


    return *this;
}

const CBM::iterator CBM::iterator::operator++(int)
{
    iterator i = (*this);
    ++(*this);
    return i;
}

bool CBM::iterator::operator==(const CBM::iterator &other) const
{
    bool matrixEquals = (this->matrix) == (other.matrix);
    bool colsEquals = this->col == other.col;
    bool rowsEquals = this->row == other.row;

    return matrixEquals && colsEquals && rowsEquals;
}

bool CBM::iterator::operator!=(const CBM::iterator &other) const
{
    return !this->operator==(other);
}



/************************************/



CBM::const_iterator::const_iterator(const CustomBasicMatrix *matrix, int row, int col) : matrix(matrix), row(row), col(col){}


CBM::const_iterator &CBM::const_iterator::operator=(const CBM::const_iterator &other)
{
    if(this == &other) return *this;

    this->matrix = other.matrix;
    this->row = other.row;
    this->col = other.col;
    return *this;

}

const int &CBM::const_iterator::operator*() const
{
    return this->matrix->operator()(this->row, this->col);
}

CBM::const_iterator CBM::const_iterator::operator++()
{

        this->col++;
        this->row += this->col / this->matrix->cols;
        this->col = this->col % this->matrix->cols;


    if(this->row == this->matrix->rows || this->col == this->matrix->cols){
        this->row = this->matrix->rows;
        this->col = this->matrix->cols;
    }

    return *this;
}

const CBM::const_iterator CBM::const_iterator::operator++(int)
{
    const_iterator i = (*this);
    ++(*this);
    return i;

}

bool CBM::const_iterator::operator==(const CBM::const_iterator &other) const
{
    bool matrixEquals = (this->matrix) == (other.matrix);
    bool colsEquals = this->col == other.col;
    bool rowsEquals = this->row == other.row;

    return matrixEquals && colsEquals && rowsEquals;
}

bool CBM::const_iterator::operator!=(const CBM::const_iterator &other) const
{
    return !this->operator==(other);
}

Edit: Added Auxiliaries header file. It is provided so the cpp file is not available, and you can ignore it in the review

Auxiliaries.h:


#include <iostream>
#include <string>

#include <cmath>

namespace sys {

    typedef int units_t;
    
    class Dimensions {
        int row, col;
    public:
        Dimensions( int row_t,  int col_t);
        std::string toString() const;
        bool operator==(const Dimensions& other) const;
        bool operator!=(const Dimensions& other) const;
        int getRow() const ;
        int getCol() const ;
    };
    
    std::string printMatrix(const int* matrix,const Dimensions& dim);

    template<class ITERATOR_T>
    std::ostream& printMatrix(std::ostream& os,ITERATOR_T begin,
                                ITERATOR_T end, unsigned int width){
        unsigned int row_counter=0;
        for (ITERATOR_T it= begin; it !=end; ++it) {
            if(row_counter==width){
                row_counter=0;
                os<< std::endl;
            }
            os <<*it<<" ";
            row_counter++;
        }
        os<< std::endl;
        return os;
    }   
}
\$\endgroup\$
1
  • \$\begingroup\$ Generally we prefer to see the contents of all the header files. If you didn't write it you can explain that and ask that it be excluded from the review. If I answer this question I will mention using #define rather than using. \$\endgroup\$ – pacmaninbw Aug 26 '20 at 19:28
5
\$\begingroup\$
  1. Instead of transposing a function by flipping dimensions, I saved a boolean value indicates if the matrix is transposed, thus when fetching the value we use said boolean to determine if we fetch (i,j) or (j,i). This is to save memory and overhead.

Unfortunately, this only increases overhead. This way you have an unnecessary branch whenever one simply want to access an element of the matrix slowing down the code and furthermore it will surely result in poor memory accessing slowing down the code even further.

Also the implementation of operations like += ignores whether the matrices are transposed. And the only way to transpose a matrix is to call transpose() which makes a copy anyways.

  1. int** data;

This isn't good. You make an allocation for each column element of the matrix - contributing a to memory fragmentation. It is preferable to store the whole data array as a contiguous piece of data, i.e., int* data; you figure out where columns begin and end via size of row. Or better wrap in an existing overhead-free smart pointer std::unique_ptr<int[]> data; and as an aside you won't need to write all the try/catch for deallocation.

  1. Why no default constructor? At least enable it. Also there is absolutely no need here to make destructor virtual - it is unnecessary for this class. Make destructors virtual for classes that have polymorphic methods.

  2. int &operator()(int row, int col); int &operator()(int row, int col) const;

I believe you wanted the const version to return either int or const int& and not int&.

  1. The class should have move constructor and move assignment operator. This is crucial for memory purposes.

  2. What does the iterator do in the matrix? What does it iterate over and in which order? It is not clear from the header. Also, the iterator should be iterating over rows instead of elements of the matrix.

CustomBasicMatrix operator<(const CustomBasicMatrix& lhs, int scalar);
CustomBasicMatrix operator<=(const CustomBasicMatrix& lhs, int scalar);
CustomBasicMatrix operator>(const CustomBasicMatrix& lhs, int scalar);
CustomBasicMatrix operator>=(const CustomBasicMatrix& lhs, int scalar);
CustomBasicMatrix operator!=(const CustomBasicMatrix& lhs, int scalar);
CustomBasicMatrix operator==(const CustomBasicMatrix& lhs, int scalar);

Is this really how you want to use the matrix? It looks odd to me. I could understand if you wanted something like this for an image... opencv uses its cv::Mat for both matrices and images but I don't find it a good design choice to wrap everything via one class.

  1. All your function implementations are in cpp file. It is not good. Functions cannot be inlined if their definition is hidden. You should move all small functions definitions to the header.

  2. I believe the primary operations of a dynamic range matrix should be multiplication by vector and computing its inverse. Both of which are missing. Though, not surprising considering that you wrote an int type matrix instead of more suitable types for a matrix like float and double.

  3. I am sure you could write a few simple functions like reserve or resize and just use them instead of copy pasting the allocation procedure. Also, copy assignment will unnecessary delete all the data even if the dimensions match.

  4. As a part of general design now in C++ viewers are pretty popular and recommended - classes that don't own memory (don't delete or allocate). A MatrixViewer class would be very efficient in wrapping things up.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks alot for the review! A few clarifications to minimize confusion: All is by following the course Regulations and Rules: 3. They asked to refrain adding a Default Constructor || 6. They explicitly asked to write an Iterator that goes through all the elements of the matrix, by order of first row to last, from left to right (as in [0,0], [0,1], ... [0,col-1], [1,0], ... [1, col-1], ... ,[row-1,0], ... [row-1, col-1] || 7. yeah they also asked for this... Over a Relational Operation with a scalar, return a 'binary' matrix that with 'true' value in each cell that satisfies the comparison. \$\endgroup\$ – Mathew Cartel Aug 26 '20 at 20:53
  • \$\begingroup\$ Nice comments! Re 8 I'd rather trust compilers and keep declarations in hh, implementations in cc files. Re 1 while in theory this may be true, but best to measure optimised code. Re 5 they can be marked deleted too if the class doesn't need to be moved around. \$\endgroup\$ – aki Aug 27 '20 at 12:09
  • \$\begingroup\$ @anki Re 8 that's the whole point that keeping implementations in cpp makes compilers unable to inline at all making this important optimization impossible. Re 5 tested in practice as well when optimising matrix/vector multiplication - though I utilised simd and doubt that I could write a decent implementation if I were to wonder what happens if matrices are transposed. \$\endgroup\$ – ALX23z Aug 27 '20 at 12:56

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.