3

In code I have been writing recently I have been forced to directly access a member of an object to call its functions, however, it feels wrong to do this because it would seem to violate encapsulation and the Law of Demeter. Yet the only good alternative I can come up with is to write my own function in the class for every single function of that member I may want to call, which would be very tedious and redundant. Example:

class Object
{
    public:
        void setNum(int x)
        {
            num = x;
        }

    private:
        int num;
};

class Object2
{
    public:
        Object obj;
};

int main()
{
    Object2 obj2;
    obj2.obj.setNum(5);
}

vs

class Object
{
    public:
        void setNum(int x)
        {
            num = x;
        }

    private:
        int num;
};

class Object2
{
    public:
        void setNum(int x)
        {
            obj.setNum(x);
        }

    private:
        Object obj;
};

int main()
{
    Object2 obj2;
    obj2.setNum(5);
}

The call to setNum in Object2 is forwarded to the same function in Object. Is such a design considered bad practice? Is accessing obj directly be any better?

I could also have Object2 inherit from Object, but in this case the class I would be inheriting from is not designed to be a base class, would expose protected members to Object2, and seems unfitting to begin with because it is not an is-a relationship and composition would be preferred.

My specific situation: I am making a game using SFML, there is a class Ship that of course needs a sprite to represent it in the world. Anytime I want to set or get the ship's position, rotation, etc. I have to either directly access its sprite or write a redundant forwarding function in Ship. The issue is that doing either one of those things seems like a code smell: either violate encapsulation and the Law of Demeter, or write redundant code.

What would be considered best practice here? Am I being overly picky about writing simple forwarding functions? Or is there really nothing wrong with directly accessing the sprite of Ship in this case?

This question: C++ Forward method calls to embed object without inheritance in fact is exactly what I'm asking, however, neither answer gives a good solution. One not being condoned and apparently having very poor encapsulation, the other simply using a getter which seems to be a placebo if anything, and no one addresses the main question I have, which is what is the most elegant and acceptable solution?

Community
  • 1
  • 1
Jeremiah Lister
  • 43
  • 6
  • 1
    The amazing thing about best practices is that everyone has different ones. – Sam Varshavchik Sep 25 '16 at 17:55
  • This is called delegation. It's generally regarded as a good pattern. It's certainly better than using inheritance for the same task. – Carcigenicate Sep 25 '16 at 17:56
  • My opinion? If something works don't try and fix it. – Krythic Sep 25 '16 at 18:00
  • @SamVarshavchik Maybe so, however, from what I've seen there are generally accepted best practices when it comes to certain issues. Books like Effective C++ and Clean Code I see recommended far and wide. I just wonder how people with large amounts of programming experience would handle this (and many other situations) and why. – Jeremiah Lister Sep 25 '16 at 19:25

2 Answers2

3

What solution is the best highly depends on underlying semantics of encapsulation. You need to decouple your code as much as possible. I'll describe that on examples:

  1. You have a Ship class and it has a Sprite. You may want to separate game logic and rendering. So all that Ships knows about the rendering is that it has a Sprite object which handles it. So in this case you are separating responsibilities and that's good. So simple getter is a good solution.
  2. But if absolute coordinates and rotation must be stored in a sprite, then things change: game logic usually needs them two, so they must be set consistently both inside a Ship and a Sprite. The best way to achieve that is to make Ship's setPosition and setRotation methods to set Sprites position and rotation too. That way you simplify the code which works with the Ship at expense of Ship complexity. Given that Ship is manipulated from several places, that's worth it. NOTE: You still may want to expose Sprite through a getter for rendering purposes. And you may want to prevent anybody except a Ship to set Sprite position and rotation, if that does not bloat your code too much.
  3. Let's imagine that Ship class is mostly devoted to rendering. In such situation you may want to hide from outer classes that you use sprites for graphics (because if you change rendering engine it will be good if you won't need to rewrite anything except rendering code). And in such situation you will want to proxy all setPosition and setRotation calls to a Sprite through Ship just to hide existence of the Sprite.

In none of this cases inheritance is used because inheritance means that child is a variation of it's ancestor. You can say that BattleShip is a variant of a Ship. But Ship is not a variant of a Sprite, they are too different and mean different things.

So: if encapsulated class is too specific and should not be visible outside or if it must be operated consistently with a master object, then writing a bunch of proxy methods is a good solution. Otherwise these methods will just bloat your code and it's better to provide a way to get nested object. But in that case I vote for a getter method instead of a public property.

Alexey Guseynov
  • 5,116
  • 1
  • 19
  • 29
  • Is it unusual to store position and rotation in the sprite? Those two things are needed for rendering and thus Sprite has them as a member, and Ship needs them for the game logic. Since Sprite already stores that information I figured it could handle the position and rotation for ship, but would it be better for ship to have its own fields too and keep them in sync? Also, is there any benefit to using a getter for the sprite rather than just making it public? If Sprite's interface changes and also if we decide to no longer use a Sprite then outside code will still be affected either way. – Jeremiah Lister Sep 28 '16 at 01:27
  • Storing position is sprite may be OK, if you can use the same coordinate system for game logic and rendering. But game logic must not know about rendering because that's example of tight coupling between systems. So there must be a way to get coordinates from the Ship directly and game logic must use it. This can be getters which take information from a Sprite. – Alexey Guseynov Sep 28 '16 at 03:45
  • Getters are better then public access because they hide how data is stored in a class. It is easier to show that on simpler objects. Let's consider Ship's position. You may store it in a ship, you may store it in a Sprite. You may store them in a special Point class with convenient modification methods. Or you may decide that you have to use different coordinate systems for rendering and game logic and do some transformations. When you use getters all these decisions do not change the API so you can change Ship's implementation without breaking things. And writing getters is relatively cheap. – Alexey Guseynov Sep 28 '16 at 04:17
1

Despite of how the classic javanese oop school can think, never forgot that also DRY (Don't Repeat Yourself) is ALSO considered a good practice.

And while OOP is just one of many programming paradigm C++ can support, DRY is the very essence of all programming since the first very assembler got macros, and this is true long before oop inventors was even far away from their own parent's thoughts and wishes.

For all what my unfair experience is... if respecting a good oop practice force you in writing useless boilerplates of repeating code it means either

  • The language you are using is fundamentally broken for the purpose you want to achieve, not supporting the paradigm correctly or...
  • OOP is broken for the purpose you are try to reach. And in C++ chances are that OOP is really the broken paradigm.

To come to your specific problem, delegation (that's how that pattern is commonly named) makes sense if:

  • the way it is delegating can be changed or
  • the delegation is to hide part of the member interface.

In you case, you have a function that calls a fixed method reachable from fixed member.

Is that only specific to this particular sample or in your implementation will be always that way by design?

If so, delegation adds no semantic value, if not just reducing a.b.m() to a.m(). If you are writing more than ... let's say three "do nothing just forward" functions you are wasting your time.

If b has 50 methods and you are making it private to delegate only 5 of them, than it makes perfectly sense.

Emilio Garavaglia
  • 20,229
  • 2
  • 46
  • 63