4
\$\begingroup\$

The goal is to have a helper class to wrap the concept of a resolution safely, encapsulating the range check into the class, since otherwise it can get lost by a client not so cautious.

I think this is all the context that's required, but an additional bit of info that may be helpful is that this looks a bit like what Rust does, where you can build restricted types out of the more common ones. Then again, I don't know Rust, and the above is a bit of a gossip that I've heard, so it may be wrong.

Technically it's just a pair of two unsigned integers of some kind (normal or long etc.), but the type is similar for both, of course. I want to enforce this as well, so that no one uses two different types by accident, thus instead of using std::pair<type, type> I use Eigen::Matrix<type, 2, 1>. Eigen is alright since it's a hard dependency of the project and I'm sure it won't go away.

Version one (I think it's suboptimal for the reasons given below):

Header:

#ifndef RESOLUTION_H
#define RESOLUTION_H

#include <Eigen/Dense>

class Resolution {
public:
    Resolution(unsigned int _x, unsigned int _y);
    static Resolution min();
    static Resolution max();
    inline unsigned int x() const { return res[0]; }
    inline unsigned int y() const { return res[1]; }
protected:
    Eigen::Matrix<unsigned int, 2, 1> res;
};

#endif

Impl:

#include <cassert>

#include "resolution.h"

Resolution::Resolution(unsigned int _x, unsigned int _y) {
    assert(_x < max().x() && _y < max().y());
    assert(_x > min().x() && _y > min().y());
    res[0] = _x; res[1] = _y;
}

Resolution Resolution::min() { return Resolution{600, 480}; }
Resolution Resolution::max() { return Resolution{3840, 2160}; }

Bad bad because unsigned int is hardcoded so many times and can get lost if someone tries to change it later. Attempt 2, fixing it:

Header:

#ifndef RESOLUTION_H
#define RESOLUTION_H

#include <Eigen/Dense>

class Resolution {
public:
    typedef unsigned int impl_type;
public:
    Resolution(impl_type _x, impl_type _y);
    static Resolution min();
    static Resolution max();
    inline impl_type x() const { return res[0]; }
    inline impl_type y() const { return res[1]; }
protected:
    Eigen::Matrix<impl_type, 2, 1> res;
};

#endif

Impl:

#include <cassert>

#include "resolution.h"

Resolution::Resolution(impl_type _x, impl_type _y) {
    assert(_x < max().x() && _y < max().y());
    assert(_x > min().x() && _y > min().y());
    res[0] = _x; res[1] = _y;
}

Resolution Resolution::min() { return Resolution{600, 480}; }
Resolution Resolution::max() { return Resolution{3840, 2160}; }

Better but naive because how can I enforce that the max and min are in the range of the type that can be used in the future? And, should I expose the type to the client (i.e. should the typedef be public or protected?)

Attempt 3, that looks like total overkill:

Header:

#ifndef RESOLUTION_H
#define RESOLUTION_H

#include <Eigen/Dense>

template<class T>
class Resolution {
public:
    typedef T impl_type;
public:
    static Resolution min() { return Resolution{0, 0}; }
    static Resolution max() { return Resolution{0, 0}; }
    inline impl_type x() const { return 0; }
    inline impl_type y() const { return 0; }
protected:
    Eigen::Matrix<impl_type, 2, 1> res;
};

template<>
class Resolution<unsigned int> {
public:
    typedef unsigned int impl_type;
public:
    Resolution(impl_type _x, impl_type _y);
    static Resolution min();
    static Resolution max();
    inline impl_type x() const { return res[0]; }
    inline impl_type y() const { return res[1]; }
protected:
    Eigen::Matrix<impl_type, 2, 1> res;
};

#endif

Impl:

#include <cassert>

#include "resolution.h"

Resolution<unsigned int>::Resolution(impl_type _x, impl_type _y) {
    assert(_x < max().x() && _y < max().y());
    assert(_x > min().x() && _y > min().y());
    res[0] = _x; res[1] = _y;
}

Resolution Resolution<int>::min() { return Resolution{600, 480}; }
Resolution Resolution<int>::max() { return Resolution{3840, 2160}; }

As said above, total overkill and code duplicaiton, but then it sort of accounts for the different underlying types that can be used.

So, to recap: - Is it worth to wait for a change of the underlying type that can occur?

  • How good is it to hardcode those constants when the type may change? When it may not change (let's suppose it never will and we assume unsigned int)?

  • Is it worth to typedef the underlying type in order to avoid duplication and prevent mistakes when it is being changed (again, what if it's never ever changed?)

  • Am I just wsting time? For some reason, I'm afraid to write code not generic at least to a degree because it looks too naive and for more practical reasons as well - I've been taught to anticipate change, though maybe in this case there is no change possible.

\$\endgroup\$
  • \$\begingroup\$ Hi, welcome to Code Review. This is a great first question and I hope you receive great answers! \$\endgroup\$ – Tunaki Mar 31 '16 at 21:23
2
\$\begingroup\$

I'd start by separating concerns. To do that, I'd design this as two types. One just restricts an arithmetic type to a range. The other holds a pair of those to form a coordinate. Restricting an arithmetic type to a range might look something like this:

template <class T, T lower, T upper>
class bounded { 
    T val;

    void assure_range(T v) {
        if (v < lower || upper <= v)
            throw std::range_error("Value out of range");
    }

public:
    bounded(bounded const &o) : val(o.val) {}

    bounded &operator=(T v) { 
        assure_range(v);
        val = v;
        return *this;
    }

    bounded(T const &v=T()) {
        assure_range(v);
        val = v;
    }

    operator T() { return val; }
};

Then a struct to hold a couple of those is trivial:

template<class T, T min_x, T max_x, T min_y, T max_y>
struct coordinate {
    bounded<T, min_x, max_x> x;
    bounded<T, min_y, max_y> y;
};

This would be instantiated something like this:

coordinate<unsigned, 640, 3840, 480, 2160> r{1024, 768};
\$\endgroup\$
  • \$\begingroup\$ Great! So I was wrong from the very beginning thinking it could be trivially coded as a single class. Guess I'm lacking knowledge of the OOP fundamentals. \$\endgroup\$ – iksemyonov Mar 31 '16 at 20:34
  • \$\begingroup\$ @iksemyonov: It certainly could be written as a single class, but I think things are simpler if you separate things a bit. \$\endgroup\$ – Jerry Coffin Mar 31 '16 at 20:38
  • \$\begingroup\$ That, by the way, is what I'm afraid the most in programming or any kind of school / thinking work for that matter: to come up with a solution only to know that the real one is actually miles away from mine. Any ideas how to cope with that? \$\endgroup\$ – iksemyonov Mar 31 '16 at 20:38
  • \$\begingroup\$ @iksemyonov: Well, I'm not entirely sure mine is "the real one". There are often multiple solutions to the same problem, and while I prefer this one, that doesn't mean it's "right" and others are "wrong". There are lots of guidelines and principles, but they conflict often enough that you typically have to make some value judgments about which matter most, so there's rarely an objective measure by which to choose the right answer. \$\endgroup\$ – Jerry Coffin Mar 31 '16 at 20:44
  • \$\begingroup\$ I've been thinking about extending this to floating-point constants. The template approach is then invalid due to the known template restrictions regarding FP constants. For FP the purpose of representing bounds, can the constants be moved into the object as two fields, initialized through the class constructor? What do you think about this approach? \$\endgroup\$ – iksemyonov Apr 3 '16 at 17:42

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.