8
\$\begingroup\$

I'm a novice at C++ in comparison with other languages, so as an exercise I wrote a square Matrix class that doesn't use STL.

#ifndef MATRIX_H
#define MATRIX_H
#include <initializer_list>
#include <algorithm>


class Matrix {
public:
  Matrix(std::size_t sz) : elements{new int[sz * sz]}, size{sz} {}
  Matrix(std::size_t sz, std::initializer_list<int> elem) : Matrix(sz) {
    std::copy(elem.begin(), elem.end(), elements);
  }
  ~Matrix() { delete[] elements; }

  /** Get the matrix element at row i and column j. */
  int get(std::size_t i, std::size_t j) const {
    return elements[convert(i, j)];
  }

  /** Set the value of the matrix at row i and column j and return the old value. */
  int set(std::size_t i, std::size_t j, int new_value) {
    std::size_t idx = convert(i, j);
    int old_value = elements[idx];
    elements[idx] = new_value;
    return old_value;
  }

  std::size_t width() const {
    return size;
  }


private:
  int *elements;
  std::size_t size;

  inline std::size_t convert(std::size_t i, std::size_t j) const {
    if (i >= size || j >= size) {
      throw std::exception("Matrix indices out of bounds");
    }
    std::size_t idx = i * size + j;
    return idx;
  }
};

#endif

Critiques are welcome! In particular, I'd be happy to get advice on the following:

  1. Is this an appropriate usage of exception handling? Is there a better option than just tossing up a std::exception if the matrix indices are out of bounds?
  2. Do the constructors and destructors make sense?
  3. Any general comments on writing in idiomatic C++ style.
\$\endgroup\$
  • \$\begingroup\$ The code doesn't compile as is. std::exception doesn't have constructor that takes std::string. Have you tried to compile and run it? \$\endgroup\$ – Incomputable Jan 30 '18 at 10:44
  • \$\begingroup\$ @Incomputable: It's a MSVC extension, but it's better to avoid using it. \$\endgroup\$ – Edward Jan 31 '18 at 13:51
  • \$\begingroup\$ @Incomputable Right, sorry about that - I was compiling using MSVC. Should have checked it with MinGW as well? \$\endgroup\$ – Axesilo Feb 2 '18 at 16:01
7
\$\begingroup\$

(1) The exception is a good way of handling such errors, but it may be a good idea to provide a non-throwing, less safe accessors. For example, std::vector::at() will throw an std::out_of_range if requested index is not valid, but std::vector::operator[] will silently accept all values. That's because, if individual elements are accessed very frequently (some complicated math algorithm), all those ifs may impact the performance. So, as it goes for the exceptions, two suggestions:

  • throw std::out_of_range instead of plain std::exception to indicate what kind of error just happened,
  • do not throw in every accessor (you only have one, which leads to point 4, see below).

(2) Yes, they do. But, the basic question is: do you really need to be able to specify size of a matrix at runtime? Perhaps it would be better to use compile-time constant size:

template <size_t Size>
class Matrix
{
private:
  int elements[Size*Size];
  //...
};

Then, you would use it this way:

Matrix<4> mat4;

In this version, no destructor would be necessary and no memory allocation required. Because of that, this version also can be faster.

(3) "Idiomatic C++ style" has become a very fragile term recently (IMHO). But, you could probably provide support for accessing elements of your matrix using operator[] (see point 4).

(4) It would be nice if users of your Matrix could write simply:

Matrix<4> mat; // Or: Matrix mat {4}, in dynamically-allocated version
mat[3][3] = 1; // set elements in row 3, column 3 (note, that indices are from 0 to 3)

Or, equivalently, in simpler version:

mat[15] = 1;

(*) Simple version only requires the presence of operator[]:

template <size_t Size>
class Matrix
{
    int& operator[](size_t index)
    {
        return elements[index];
    }

    const int& operator[](size_t index) const
    {
        return elements[index];
    }
};

Note, that two version are provided, so you can do this:

Matrix<4> mat;
mat[5] = 1;

but not this:

const Matrix<4> constMat;
constMat[2] = 3; // compilation error: cannot assign to const reference

(**) The more elegant, but also more difficult version (nested [][] syntax), requires, that operator[] of the matrix must return an object (or an array) that also supports [] notation.

The easier way to achieve this would be to to change the T* elements to T** elements (or, in templated version, elements[Size*Size] to elements[Size][Size]). Note, that in this case you do not need the convert() function, since you can use the indices explicitly.

You could also declare simple Vector struct (I will limit myself here to templated version):

template <size_t Size>
struct Vector
{
private:
  int elements[Size];

public:
  int& operator[](size_t index)
  {
    return elements[index];
  }

  const int& operator[](size_t index) const
  {
    return elements[index];
  }
};

And then implement your Matrix using Vector:

template <size_t Size>
class Matrix
{
private:
  Vector<Size> columns[Size];

public:
  Vector<Size>& operator[](size_t index)
  {
    return columns[index];
  }

  const Vector<Size>& operator[](size_t index) const
  {
    return columns[index];
  }
};

(1.1) Now, after we covered the topic of operator[], we can return to the exceptions once more. To aid those unsafe operator[]s, we can add more safe, explicit at() function:

template <size_t Size>
class Matrix
{
private:
  Vector<Size> columns[Size];

public:
  Vector<Size>& at(size_t index)
  {
    if (index >= Size) throw std::out_of_range("Invalid index!");
    return columns[index];
  }

  const Vector<Size>& at(size_t index) const
  {
    if (index >= Size) throw std::out_of_range("Invalid index!");
    return columns[index];
  }
};
\$\endgroup\$
  • \$\begingroup\$ Good work. Though there is a much easier way to implement multi-index accesse. Simply store a reference to the Matrix and the first index in an object. \$\endgroup\$ – Martin York Jan 30 '18 at 19:01
  • \$\begingroup\$ "Idiomatic C++ style" has become a very fragile term recently what do you mean by this. \$\endgroup\$ – Martin York Jan 30 '18 at 19:03
  • \$\begingroup\$ The idea of having a checked at and unchecked [] really appeals to me. Also, the idea of making the size a compile-time constant using templating is a new one for me, although I guess that's what std::array<N> does. Thanks very much. \$\endgroup\$ – Axesilo Feb 1 '18 at 0:00
  • 1
    \$\begingroup\$ @Axesilo Yes, std::array<T, N> is a simple wrapper for a compile-time constant-sized array of N objects of type T. You can, of course, use the original concept with dynamic allocation, but for most usage cases it is not needed, since you usually know the required matrix dimensions in advance. And keeping the data within the object improves speed and cache usage, since additional fetch from location pointed to by the internal data pointer is not required anymore. \$\endgroup\$ – Mateusz Grzejek Feb 1 '18 at 11:40
2
\$\begingroup\$

Questions

Is this an appropriate usage of exception handling?

No. You should have the option of having a checked access. But in most use cases you have already checked the range. Thus forcing a check on each accesses becomes superfluous.

Standard use case might look like this:

for(int loopX = 0; loopX < m.width(); ++loopX) {
    for(int loopY = 0; loopY < m.width(); ++loopY) {
        std::cout << m.get(loopX, loopY) << " ";
    }
}

In this situation you know that both loopX and loopY are guaranteed to be in the correct range. So why is there an un-needed test to see if they are in the correct range.

But even std::vector has a checked version of member accesses. See vector::at(). Though the standard member accesses is not checked.

Is there a better option than just tossing up a std::exception if the matrix indices are out of bounds?

When you do check four out of range situations you could use std::out_of_range

Do the constructors and destructors make sense?

Sure. As far as they go.
But you have made the biggest beginner mistake in not implementing the rule of three. As a result you have opened yourself up to a lot of potential problems with deleting the matrix.

Matrix   x(5);     // Your constructor.
Matrix   y(x);     // Compiler generated constructor.
                   // This will compile. But because your class contains
                   // a RAW owned pointer (you delete the pointer) your
                   // class is not going to work as expected when it goes
                   // out of scope.

Look up the rule of 3 and implement that.

Any general comments on writing in idiomatic C++ style.

  1. Rule of Three
  2. Don't make things expensive for expert users just to hold the hand of the beginner.
  3. Implement Move Semantics (moving is cheaper than copying).
  4. Use [] operator rather than get/set (though you can have both).
  5. Think about how your class will be used with the standard library.
    Can you add iterators to it (that gives you accesses to algorithms).
  6. Have you though about standard matrix operations and how they can be optimized.

Design

Accessing matrix elements is usually done with [] in maths. To make your matrix easy to use by other maths like people maybe you should implement this (or something close).

Simple V1 technique:

 int&  operator()(int x, int y);   // Not [] but a close relative
                                   // It allows a simple way to use 2
                                   // dimensions with little work.

 // Don't forget the const version
 int const& operator()(int x, int y) const;

Alternatively harder the actual [] operator. The problem here is that [] can only take one index. So you need an intermediate object to do this correctly. @Mateusz Grzejek Did a good first version. But its a bit inefficient as it requires a copy of a line.

 // A quick way to do it simply.
 // You will need to add the const versions of stuff yourself.
 class Matrix
 {
     private:
        int& access(int x, int y);
        int& operator()(int x, int y) {return access(x, y);}

        // Row is cheap to create and copy.
        // Allows you to get a reference to a row and then
        // access an element in the Row.
        class Row {
            Matrix* parent;
            int     x;
            public:
                Row(Matrix* p, int x)
                    : parent(p)
                    , x(x)
                {}
                int& operator[](int y) {return parent->access(x, y);}
        };
        Row operator[](int x){return Row(this, x);}
  };

Code Review

Nothing special to say.

\$\endgroup\$
  • 1
    \$\begingroup\$ The rule of 3 has become the rule of 5 since C++11. \$\endgroup\$ – Cris Luengo Jan 31 '18 at 1:12
  • \$\begingroup\$ @CrisLuengo If the final version is the one with dynamically-allocated memory, move semantics will be quite important. But in case of fixed, compile-time sized storage (template-based solution), it won't be necessary, since you can't move such storage around. \$\endgroup\$ – Mateusz Grzejek Jan 31 '18 at 9:30
  • \$\begingroup\$ @MateuszGrzejek: I don't think I understand you. If you need a destructor, you likely need to customize both copy and move, or at least prohibit them by explicitly deleting the relevant operators and constructors. If your class doesn't manage any resources, then there is no need for move semantics, but also not for copy semantics or a destructor. \$\endgroup\$ – Cris Luengo Jan 31 '18 at 14:38
  • \$\begingroup\$ @CrisLuengo: The rule of three still exists. This is because if you define the copy operations the move operations are disabled by default. The rule of five is an addition to the rule of three (not a replacement). So it is the rule of 3 and/or 5. \$\endgroup\$ – Martin York Jan 31 '18 at 17:21
  • 1
    \$\begingroup\$ @Axesilo: Correct. (And Copy Assignment). \$\endgroup\$ – Martin York Feb 2 '18 at 18:43

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.