3
\$\begingroup\$

I make a 2D matrix class (where matrix elements are of type float) which so far can:

  • Create a matrix of zeros of any size by typing Matrix2D myMatrix(n,m) where n is row size and m is column size (default is 1)
  • Create a matrix based on an initializer list, for example Matrix2D myMatrix({{1,2},{3,4}}) creates the matrix

$$ \texttt{myMatrix} = \begin{bmatrix} 1 & 2 \\ 3 & 4 \end{bmatrix} $$

  • Delete row i of a matrix by typing myMatrix.removeRow(i)
  • Delete column i of a matrix by typing myMatrix.removeColumn(i)
  • Concatenate two Matrix2D matrices A and B horizontally by typing A.horzcat(B)
  • Concatenate two Matrix2D matrices A and B vertically by typing A.vertcat(B)
  • Do matrix equality by overloading the = operator
  • Access a matrix element by overloading the () operator, e.g. A(i,j)
  • Do matrix addition by overloading the + operator

Here is my code:

#ifndef _MATRIXCLASS2_HPP
#define _MATRIXCLASS2_HPP

// System includes
#include <iostream>
#include <vector>
#include <cstdio>

// Personal includes
#include "exceptionClass.hpp"

using namespace std;

/*******************************
 * Matrix class: definition
 *******************************/

typedef float type;
typedef vector<vector<type> > vector2D;
typedef vector<type> vector1D;

class Matrix2D {
private:
    vector2D _matrix; // the matrix itself, a two-dimensional vector
public:
    /* Constructors */
    Matrix2D(size_t numRows=1, size_t numCols=1) : _matrix(vector2D(numRows, vector1D(numCols))) {} // zero matrix
    explicit Matrix2D(const initializer_list<initializer_list<type> > & matrixAsAList) {
    // matrix given by brace enclosed initializer list, e.g. {{1,2},{3,4}}
    _matrix.assign(matrixAsAList.begin(), matrixAsAList.end());
    }
    Matrix2D(const Matrix2D & matrixToCopy) : _matrix(matrixToCopy._matrix) {}

    /* Getters */
    vector2D fullMatrix() const { return _matrix; } // output the full matrix
    size_t numRows() const { return _matrix.size(); }
    size_t numColumns() const { return _matrix[0].size(); }
    void print() const;

    /* Setters */
    void removeRow(size_t);
    void removeColumn(size_t);
    void horzcat(const Matrix2D &); // horizontal matrix concatenation
    void vertcat(const Matrix2D &); // vertical matrix concatenation

    /* Operator overloads */
    type & operator () (size_t, size_t);
    Matrix2D & operator = (const Matrix2D &);
    Matrix2D & operator + (Matrix2D &);
};

// Print the whole matrix
void Matrix2D::print() const {
    for (size_t i=0; i<numRows(); i++) { // iterate over rows
    printf("[ ");
    for (size_t j=0; j<numColumns(); j++) { // iterate over columns
        printf("%.3f ",_matrix[i][j]);
    }
    printf("]\n");
    }
}

// Delete row
void Matrix2D::removeRow(size_t row) {
    if (row>=numRows()) {
    throw E("Row number for deletion is out of range, not going to delete anything");
    } else {
    _matrix.erase(_matrix.begin()+row); // delere row (NB: .erase() decrements both size and capacity)
    }
}


// Delete column number columNumber
void Matrix2D::removeColumn(size_t column) {
    if (column>=numColumns()) {
    throw E("Column number for deletion is out of range, not going to delete anything");
    } else {
    for (size_t i=0; i<numRows(); i++) { // iterate over rows
        _matrix[i].erase(_matrix[i].begin()+column); // delete element in column
    }
    }
}

// Horizontally concatenate matrix with another matrix, matrix2
void Matrix2D::horzcat(const Matrix2D & matrix2) {
    if (numRows() != matrix2.numRows()) {
    throw E("Row sizes do not correspond, cannot concatenate matrices!");
    } else {
    for (size_t i=0; i<numRows(); i++) {
        // reserve necessary space (reserve throws length_error exception if unable to do so)
        _matrix[i].reserve(_matrix[i].size()+matrix2._matrix[i].size());
        // append matrix2 row i to end of matrix row i
        _matrix[i].insert(_matrix[i].end(), matrix2._matrix[i].begin(), matrix2._matrix[i].end());
    }
    }
}

// Vertically concatenate matrix with another matrix, matrix2
void Matrix2D::vertcat(const Matrix2D & matrix2) {
    if (numColumns() != matrix2.numColumns()) {
    throw E("Columnn sizes do not correspond, cannot concatenate matrices!");
    } else {
    // reserve necessary space (reserve throws length_error exception if unable to do so)
    _matrix.reserve(_matrix.size()+matrix2._matrix.size());
    // append matrix2 to the bottom of matrix
    _matrix.insert(_matrix.end(), matrix2._matrix.begin(), matrix2._matrix.end());
    }
}

// Overload (), get element at row and column of _matrix
type & Matrix2D::operator () (size_t row, size_t column) {
    return _matrix.at(row).at(column);
}

// Overload =
Matrix2D & Matrix2D::operator = (const Matrix2D & rhs) {
    if (this != &rhs) {
    _matrix = rhs._matrix;
    }
    return *this;
}

// Overload + (matrix addition)
Matrix2D & Matrix2D::operator + (Matrix2D & rhs) {
    // compute result = matrix + rhs
    static Matrix2D result; // initialize the result in static storage (safer & more efficient)
    result = *this; // copy matrix intro result
    if (rhs.numRows() != numRows() || rhs.numColumns() != numColumns()) {
    // throw error if rhs column or row size does not match matrix
    throw E("Row of column size mismatch, won't add matrix.");
    }
    // perform matrix addition
    for (size_t i=0; i<numRows(); i++) {
    for (size_t j=0; j<numColumns(); j++) {
        result(i,j) += rhs(i,j);
    }
    }
    return result;
}

#endif // _MATRIXCLASS2_HPP

The exceptionClass.hpp header is pretty simple:

#ifndef _EXCEPTIONCLASS_H
#define _EXCEPTIONCLASS_H

#include <exception>

class E: public std::exception {
private:
    const char * message = nullptr;
    E(){}; // such a constructor not possible!
public:
    explicit E(const char * s) throw() : message(s) {}
    const char * what() const throw() { return message; }
};

#endif // _EXCEPTIONCLASS_H

This is my first ever object-oriented project. I'm looking for any advice on how I can improve my code in terms of efficiency/readability/portability. Thank you!

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$
  1. It's generally advisable to keep your implementation code in *.cpp file and not in the header.
  2. using namepsace should never occur in a header file. It will force users of your class to import names from the namespace, which they potentionally do not want to.
  3. Use descriptive name for your exception. Probably you want two different names - for deletion and for concatenation. Propably you also should inherit from std::range_error.
  4. Do not use throw() exception specification. It is deprecated. Use noexcept instead.
  5. You probably want to implement move assignment and move constructor (as well as copy constructor). It's a rule of thumb - if you implement one, you should implement others, as it helps with usablity and efficience of you code. Also, if you have decent compiler, fully implementing c++11 standard, it will generate these functions for you. However, it is considered good practice to declare them explicitly:Matrix2D(Matrix2D&&) = default;, etc.
  6. static Matrix2D result; // initialize the result in static storage (safer & more efficient). It's niether save nor particulary efficient. In multithreaded environment result will be a subject to a data race, with operator+ leading to undefined behavior. Move constructors and assignments were created exactly to fight unnecessary copies. Just create result matrix on stack and return it, memory owned by vectors will be transferred to recipient.
  7. Mark unwanted functions ( as exception constructor ) with = delete annotation.
  8. Code identation inside curly braces is not consistent. I recommend pick a style and use for all the code.
  9. In concatenation functions there is no need to call reserve() before insert() as it is guaranteed to be called inside insert(). Input vector iterators can be used to extract information of size of inserted data, so insert() make use of this information and do reserve() itself.
  10. (NB: .erase() decrements both size and capacity). It is not required to decrement capacity, and as far as I know, it does not for all major compilers. This can seem to be picky, but what I want to show here is that the code have abundance of comments. And comments tend to become wrong and outdated over time. Comments can even be wrong as they written as compiler can not verify them, and test suits won't catch errors in comments. I recommend to use the least amount of comments possible, only essential ones.
\$\endgroup\$
1
  • \$\begingroup\$ Thank you! I refined my code with your comments, but I'm still a beginner in C++ and, if you're able, please elaborate points 3, 4 and 5. I don't understand what I have to do there - if you can, give concrete examples for my code. Thanks again :) \$\endgroup\$ Commented Oct 12, 2016 at 16:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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