Functions that simply call another function, bad design choice?

Question

I have a setup of a class that represents a building. This building has a floor plan, which has bounds.

The way I have it setup is like this:

public struct Bounds {} // AABB bounding box stuff

//Floor contains bounds and mesh data to update textures etc
//internal since only building should have direct access to it no one else
internal class Floor {  
    private Bounds bounds; // private only floor has access to
}

//a building that has a floor (among other stats)
public class Building{ // the object that has a floor
    Floor floor;
}

These objects have their own unique reasons to exist as they do different things. However there is one situation, where I want to get a point locally to the building.

In this situation I am essentially doing:

Building.GetLocalPoint(worldPoint);

This then has:

public Vector3 GetLocalPoint(Vector3 worldPoint){    
    return floor.GetLocalPoint(worldPoint);
}

Which leads to this function in my Floor object:

internal Vector3 GetLocalPoint(Vector3 worldPoint){
    return bounds.GetLocalPoint(worldPoint);
}

And then of course the bounds object actually does the math required.

As you can see these functions are pretty redundant as they just pass on to another function lower down. This doesn't feel smart to me - it smells like bad code thats going to bite me in butt some where down the line with code mess.

Alternatively I write my code like below but I have to expose more to public which I kinda don't want to do:

building.floor.bounds.GetLocalPoint(worldPoint);

This also starts to get a bit silly when you go to many nested objects and leads to large rabbit holes to get your given function and you may end up forgetting where it is - which also smells like bad code design.

What is the correct way to design all this?

Answer 1

Never forget the Law of Demeter:

The Law of Demeter (LoD) or principle of least knowledge is a design guideline for developing software, particularly object-oriented programs. In its general form, the LoD is a specific case of loose coupling. The guideline was proposed by Ian Holland at Northeastern University towards the end of 1987, and can be succinctly summarized in each of the following ways:[1]

• Each unit should have only limited knowledge about other units: only units "closely" related to the current unit.
• Each unit should only talk to its friends; don't talk to strangers.
• Only talk to your immediate friends.

The fundamental notion is that a given object should assume as little as possible about the structure or properties of anything else (including its subcomponents), in accordance with the principle of "information hiding".
It may be viewed as a corollary to the principle of least privilege, which dictates that a module possess only the information and resources necessary for its legitimate purpose.

---

building.floor.bounds.GetLocalPoint(worldPoint);

This code violates the LOD. Your current consumer somehow is required to know:

• That the building has a floor
• That the floor has bounds
• That the bounds have a GetLocalPoint method

But in reality, your consumer should only be handling the building, not anything inside of the building (it shouldn't be handling the subcomponents directly).

If any of these underlying classes change structurally, you're suddenly also required to change this consumer, even though he may be several levels up from the class you actually changed.
This starts infringing on the separation of layers you have, as a change affects multiple layers (more than just its direct neighbors).

public Vector3 GetLocalPoint(Vector3 worldPoint){    
    return floor.GetLocalPoint(worldPoint);
}

Suppose you introduce a second type of building, one without a floor. I can't think of an real-world example, but I'm trying to show a generalized use case, so let's assume that EtherealBuilding is such a case.

Because you have the building.GetLocalPoint method, you are able to change its workings without the consumer of your building being aware of it, e.g.:

public class EtherealBuilding : Building {
    public Vector3 GetLocalPoint(Vector3 worldPoint){    
        return universe.CenterPoint; // Just a random example
    }
}

What's making this harder to understand is that there is no clear use case for a building without a floor. I don't know your domain and I can't make a judgment call on if/how that would occur.

But development guidelines are generalized approaches that forgo specific contextual applications. If we change the context, the example becomes clearer:

// Violating LOD

bool isAlive = player.heart.IsBeating();

// But what if the player is a robot?

public class HumanPlayer : Player {
    public bool IsAlive() {
        return this.heart.IsBeating();
    }
}

public class RobotPlayer : Player {
    public bool IsAlive() {
        return this.IsSwitchedOn();
    }
}

// This code works for both human and robot players, and thus wouldn't need to be changed when new (sub)types of players are developed.

bool isAlive = player.IsAlive();

Which proves the point why the method on the Player class (or any of its derived classes) has a purpose, even if its current implementation is trivial.

---

Sidenote
For the sake of example, I've skirted a few tangential discussions, such as how to approach inheritance. These are not the focus of the answer.

Answer 2

If you have such methods occasionally here and there, it may just be a side effect (or price to pay, if you will) of a consistent design.

If you have a lot of them then I would consider it a sign that this design is itself problematic.

In your example, maybe there shouldn't be a way to "get a point locally to the building" from outside the building and instead the building's methods should be at a higher level of abstraction and work with such points only internally.

Answer 3

The famous "Law of Demeter" is a law that dictates what kind of code to write, but it does not explain anything useful. Flater's answer is fine because it gives examples, but I would not call these "violation of/compliance with the law of Demeter". If the "Law of Demeter" is enforced where you are, please get in touch with your local Demeter Police Station, they will be happy to sort out the issues with you.

Remember that you are always master of the code you write, and therefore, between creating "delegating functions" and not writing them, it is a matter of your own judgement. There is no crisp line, so no one sharp rule can be defined. On the contrary, we can find cases, like Flater did, where creating such functions are plain useless, and where creating such functions are useful. (Spoiler: In the former case, the fix is to inline the function. In the latter, the fix is to create the function)

Examples where it is useless to define a delegating function include when the sole reason would be:

• To access a member of an object returned by a member, when the member is not an implementation detail that should be encapsulated.
• Your interface member is correctly implemented by .NET's quasi-implementation
• To be Demeter-compliant

Examples where it is useful to create a delegating function include:

• Factoring out a call chain that is repeated over and over
• When the language forces you to, e.g. implement an interface member by delegating to another member or simply calling another function
• When the function you call is not at the same conceptual level as the other calls at the same level (e.g. a LoadAssembly call at the same level than plugin introspection)

Answer 4

Forget that you know the implementation of Building for a moment. Someone else has written it. Maybe a supplier who only gives you compiled code. Or some contractor who actually starts writing it next week.

All you know is the interface for Building and the calls you make to that interface. They all look quite reasonable, so you are fine.

Now you put on a different coat and suddenly you are the implementor of Building. You don't know the implementation of Floor, you just know the interface. You use the Floor interface to implement your Building class. You know the interface for Floor and the calls you make to that interface to implement your Building class, and they all look quite reasonable, so you are fine again.

All in all, no problem. Everything is fine.

Answer 5

building.floor.bounds.GetLocalPoint(worldPoint);

is BAD.

Objects should only deal with their immediate neighbors because your system will be VERY hard to change otherwise.

Answer 6

It is OK to just call functions. There are event many design patterns which are using that technique, for example adapter and facade but also to some extend patterns like decorator, proxy and many more.

It's all about levels of abstractions. You should not mix concepts from different levels of abstractions. To do so you need sometimes to make call to inner objects so that your client will not be forced to do it himself.

For example (Car example will be simpler):

You have Driver, Car and Wheel objects. In real world, in order to drive a car, do you have driver doing something directly with wheels or does he only interact with car as a whole?

How to know that something is NOT ok:

• Encapsulation is broken, inner objects are available in public API. (e.g. code like car.Wheel.Move()).
• SRP principle is broken, objects are doing many different things (e.g. preparing e-mail message text and actually sending it in same object).
• It is difficult to unit test particular class (e.g. there are many dependencies).
• There are different domain experts (or company departments) handling things which you handle in same class (e.g. sales and package delivery).

Potential problems when breaking Law of Demeter:

• Hard unit testing.
• Dependency on the internal structure of other objects.
• High coupling between objects.
• Exposing internal data.