1
\$\begingroup\$

I know that other people already did the same already, but I'm learning C++ (C++11/14) coming from C (I'm reading Programming: Principles and Practice Using C++, by Bjarne Stroustrup), and this is my OOP implementation of a matrix (N by M size).

At the moment it doesn't use template for generic types and uses double, and all it does is represent a matrix, but doesn't implement any calculation function members yet. I'm using the C++ PIMPL idiom (using C++11's unique_ptr and C++14's make_unique), and the subscript [] operator overload.

I'd like to know if I'm using a good programming pattern (or a bad antipattern) and in the correct C++ way of doing things (ex. program structure, files and organization, types etc...). Please if you can keep it simple, as I'm still missing parts of the language I haven't learned yet.

File main.cpp:

#include <iostream>
#include <memory>

#include "Matrix.hpp"

using namespace std;
using namespace zgt;

constexpr size_t num_elements{ 3};

/*
 Outputs:
 1, 2, 3
 4, 5, 6
 7, 8, 9
 */
int main() {
    Matrix m{ num_elements }; // Squared matrix

    double inc{ 1 };
    for (size_t i{ 0 }; i < num_elements; i++) {
        for (size_t j{ 0 }; j < num_elements; j++) {
            m[i][j] = inc++;
        }
    }

    for (size_t i{ 0 }; i < num_elements; i++) {
        for (size_t j{ 0 }; j < num_elements; j++) {
            if (j == 0) {
                cout << m[i][j];
            } else {
                cout << ", " << m[i][j];
            }
        }

        cout << endl;
    }

    return 0;
}

File Matrix.cpp:

#include "Matrix.hpp"

using namespace std;
using namespace zgt;

struct Matrix::_impl {
    unique_ptr<unique_ptr<MatrixRow>[]> _m;

    _impl(size_t rows, size_t cols): _m(make_unique<unique_ptr<MatrixRow>[]>(rows)) {
        for (size_t i{ 0 }; i < cols; i++) {
            _m[i] = make_unique<MatrixRow>(cols);
        }
    }
};

MatrixRow& Matrix::operator[](size_t index) {
    return *(pimpl->_m[index]);
}

Matrix::Matrix(size_t rows, size_t cols) : pimpl(make_unique<_impl>(rows, cols)) {}
Matrix::Matrix(size_t size) : Matrix::Matrix(size, size) {}
Matrix::~Matrix() = default;

File Matrix.hpp:

#ifndef Matrix_hpp
#define Matrix_hpp

#include <iostream>
#include <memory>

#include "MatrixRow.hpp"

namespace zgt {
    class Matrix{
        struct _impl;
        std::unique_ptr<_impl> pimpl;
    public:
        Matrix(size_t rows, size_t cols);
        Matrix(size_t size); // [size][size]
        ~Matrix();

        MatrixRow& operator[](size_t index);
    };
}

#endif

File MatrixRow.cpp

#include "MatrixRow.hpp"

using namespace std;
using namespace zgt;

struct MatrixRow::_impl {
    unique_ptr<double[]> _v;

    _impl(size_t cols): _v(make_unique<double[]>(cols)) {}
};

double& MatrixRow::operator[](size_t index) {
    return pimpl->_v[index];
}

MatrixRow::MatrixRow(size_t cols) : pimpl(make_unique<_impl>(cols)) {}
MatrixRow::~MatrixRow() = default;

File MatrixRow.hpp:

#ifndef MatrixRow_hpp
#define MatrixRow_hpp

#include <iostream>
#include <memory>

namespace zgt {
    class MatrixRow{
        struct _impl;
        std::unique_ptr<_impl> pimpl;
    public:
        MatrixRow(size_t cols);
        ~MatrixRow();

        double& operator[](size_t index);
    };
}

#endif
\$\endgroup\$
  • 1
    \$\begingroup\$ For those who wonder what's going on in this code: it uses pointer to implementation. I think that indirection could be omitted, because it won't cause big compilation time increase (which is what pimpl prevents). Absence of templates makes it even more lightweight. \$\endgroup\$ – Incomputable Feb 24 '17 at 16:47
  • \$\begingroup\$ @Incomputable I already edited the code to say I'm using PIMPL, I was forgetting to mention it \$\endgroup\$ – Zorgatone Feb 24 '17 at 16:47
  • \$\begingroup\$ Actually, I'm trying out that idiom, assuming this implementation will grow, and to keep private data outside of the header \$\endgroup\$ – Zorgatone Feb 24 '17 at 16:48
  • 1
    \$\begingroup\$ Well, pimpl adds indirection which slightly increases complexity for humans. It also adds indirection for compiler, which probably prevents it from inlining functions, and inlining is a base for all other optimizations. Though I might be too biased on performance sometimes. \$\endgroup\$ – Incomputable Feb 24 '17 at 16:57
3
\$\begingroup\$

Lets start from the usage:

  • It is not possible to write range loops with the current version

  • no rowcount() and columncount() (or anything similar). So it is not possible to actually traverse matrix

  • operator[]() won't work on const objects, since there is no operator[](std::size_t index) const.

Clarity and cleanliness:

  • nuke pimpl. I'm serious, it doesn't add much benefit here. In fact, the compilation might be increased by pimpl, since matrix is very lightweight anyway.

  • #include <iostream> header is not needed, should be removed.

  • double indirection. _impl should actually be MatrixRow. But if pimpl is gonna get nuked, it will be embedded into matrix itself.

  • std::vector<> would be much better fit here.

    Nitpicks:

  • destructor is generated for you automatically. defaulting it is redundant.

Some general guidelines:

  • OOP is usually mixed with some functional programming in C++. For example, templates.

  • C++ code usually tries to provide some default behavior that is correct and safe, and some access to underlying implementation that when accessed lets programmer to take all of the control to himself (literally everything).

Recommendation for next iteration:

I think that std::vector<T> will be the best fit here. Calculating the index to jump at should be pretty easy (it is what compiler does for us anyway). I think it will be okay to return T* from operator[](), because users know they shouldn't mess with it, but making some small struct would good as well. Something like std::array_view<T> would be nice (though it seems like it is not in the standard yet, even in C++17). Also, you get iterators for free with vector approach.

Just write:

auto begin()
{
    return storage.begin();
}

auto end()
{
    return storage.end();
}

As a result, you get range loop for free as well.

\$\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.