CODEWITHSUNDEEP
c++oop
Jeremiah Lister
Jeremiah Lister

Reputation: 53

Is forwarding function calls to a member bad practice?

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?

Upvotes: 3

Views: 890

Answers (2)

Emilio Garavaglia
Emilio Garavaglia

Reputation: 20730

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.

Upvotes: 1

Alexey Guseynov
Alexey Guseynov

Reputation: 5304

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.

Upvotes: 3

Related Questions