Coding Style – Are Functions That Simply Call Another Function a Bad Design Choice?

clean codecode-qualitycoding-styledesign

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?

Best Answer

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.

Related Topic