How to know where to draw the line between "safe" code and "over-engineered" code?

Question

Suppose I have a class Texture that will be passed to a Renderer to be displayed on screen. One possible design is as follows:

class Texture
{
    public:
    Texture(unsigned w, unsigned h) : w_ {w}, h_ {h}, buf_(w*h) {}

    void set_pixel(size_t i, Color c) { ... }
    std::vector<Color> image_data() const { ... }
    unsigned width() const { ... }
    unsigned height() const { ... }

    private:
    unsigned w_, h_;
    std::vector<Color> buf_;
};

This design is "safe". The buf_ vector will never be uninitialized and the implementation details are hidden. On the other hand, this design:

struct Texture
{
    std::vector<Color> buf;
    unsigned w {}, h {};
};

is way more simple. In my case especially, when I find myself only using it once or twice, it's hard for me to decide whether or not I should keep it simple as in the second case, or write "proper" code as in the first case.

Answer 1

Have you considered why you have named your class/struct Texture? The keywords are meaning, concept, intention...

Consider your struct Texture design to be a razorblade, it cuts extremely well (no compilation overhead, no initialization overhead, you have full control). If you are going to be "juggling" a lot of those, you increase the risk of hurting yourself (by null pointer exceptions or garbage values from uninitialized members, among other things). The Texture structure has a bug generation risk factor of, say, 0.05% of the time. It is also not that self-explanatory, so it is a deceptively good-looking razor. When you see it, you don't realize how dangerous it may be if you are going to handle it. The more you play with it, chances you will get cut rise very quickly. Finally, you don't really understand very quickly what its potential usefulness might be to you.

The class Texture design also cuts (to the heart of the problem, no less), and it cuts well. This is more like a kitchen knife, with a handle. It has a bug generation risk factor that is much smaller, say 0.001% of the time, because now, you are your only problem, the class users will only touch the knife from its handle (its public API, your declared public methods/members only), while you are free to sharpen the blade (its private members) in all secrecy, behind the scenes. Furthermore, through its methods, its intended meaning/functionality is clearer, so it is a shiny silver kitchen knife, you know one when you see it (knife blades are almost always silver, like most image textures are made of pixels, though this is also a different matter of discussion). You can play with it much longer before any serious bug may come up.

In short, your solution is over-engineered when you are not going to exploit the designed capabilities to the maximum possible extent. You will have to get the feeling with time, how to choose the right tools for the job, but, more often than not, it depends on the job. And that's why we don't use scalpels to slice bread, and surgeons don't use kitchen knives to perform surgery.

By the way

it's hard for me to decide whether or not I should keep it simple as in the second case, or write "proper" code as in the first case.

Your second case is not simple, it is excessively simple. There is a threshold while travelling down the complexity ladder, beyond which a design becomes bad. Keeping it simple was never meant to include dropping fundamentals, a good constructor of an object certainly being one.

Answer 2

This cannot be answered objectively.

If you only use the code yourself, the simple approach may be okay. Yet if you ever forget to to initialize it correctly and this leads to a subtle bug which takes you hours or days to find, you might change your mind about that.

If you are working on a team, can you rely upon all other team members to use your struct correctly and do you really want to rely on that? If not, you better go for the safe approach.

If that shall be part of an API that you will give out and that will be used by people you won't even know, then you should definitely take the safe approach.

In your case its also a question about performance and memory consumption. Not in this particular case but textures and renders sound like 3D graphics processing and here memory consumption and speed is not irrelevant. Often 3D APIs or engines treat safety for performance as the safety is only beneficial for developers, end users won't care if the code was safe as long as it works correctly, is fast, and uses little resources. Making the code correct is more work for developers if it isn't safe but again, the end user won't care, unless this affects the price of the product.

Personally, I prefer the safe approach unless performance forces me to use an unsafe one. Why? Simple: I don't trust myself in this aspect. Considering the amount of code I write, I won't remember the details anymore in two years, so it only helps myself if I only have to deal with simple interfaces and not remember nifty details like "Oh, and you have to first initialize x in case you have set y, otherwise you must set a value to z", instead I call my construtor and it will do that correctly for me, as I had this knowledge when I wrote the code but it might be lost till I use it again in the future. I could document everything or write tons of comments but how is that less work than making it safe?

Answer 3

For most situations like this the question is whether the possible class has significant invariants it can maintain, and the extra work you're putting in will maintain those invariants.

At least in my experience, if there are invariants you can maintain, chances are pretty good that writing the code to do so will be justified in all but the most thoroughly trivial of cases.

At the same time, pseudo-classes and quasi-classes have been known for decades, but continue to be widely seen even today. The paper gives this example:

class Thing {
private:
    long value;
public:
    Thing(const long x = 0) : value(x) {}
    Thing(const Thing& t) : value{t.value) {}

    ~Thing() {}

    Thing& operator=(const Thing& rs)
    {
        value = rs.value;
        return *this;
    } 

    long getValue() const
    {
        return value;
    }

    void setValue(const long x))
    {
        value = x;
    }
};

In this case, we see a lot of code--and it's all fairly harmless, but ultimately pointless. The getter just retrieves the value as-is. The setter just sets whatever value you gave it. Defining Thing a; doesn't provide anything that long a; wouldn't do just as well (and possibly better). Yet many people see this a somehow being "more object oriented", and a path to object oriented righteousness (or something on that order).

So, the question ultimate is what the user gets out of the more complex code. If you're really making their life simpler, then it's probably justified. And yes, in some cases making their life simpler means taking something way, not adding something new.

Just for example, it can be entirely reasonable to define a class specifically to reduce the available operations to a small set that's easily reasoned about so you can be much more certain that its client code won't go wrong by trying to use operations that may work, but are difficult (or even impossible) to be really certain about.

Reference

1. Pseudo-classes and quasi-classes Confuse Object-Oriented Programming