Dots and Double Dots

I love the phrase "train wreck". To clarify, I'm not talking about real life transport accidents here. I'm talking about the phrase used to describe code.

I think I first discovered the phrase in Code Complete, though I can't pin down the exact quote.

Basically it refers to code that refers to members of members of members of members... so for example

Ammo GetCurrentAmmo()
{
 PlayerManager.GetPlayer(1).GetInventory().GetWeapons()[0].GetAmmo();
}

(I guess the dots "." are the coupling between carriages and the method names are the carriages. Or something).

The phrase seems to say "some terrible disaster has happened here". When there are that many decompositions in a single line of code, if the disaster hasn't already happened yet, it soon will.

One accident (or several) waiting to happen here is - one of the functions along the way could return a null pointer. If any of the intermediate elements is an invalid value then this piece of code is going to crash.

So at the very least, we should be testing for null values. Now, say we wanted the inventory of player 1, and assuming we know that the PlayerManager is valid, one way of writing this (with null value testing included) is:

if (PlayerManager.GetPlayer(1) != null)
   inv = PlayerManager.GetPlayer(1).GetInventory();

However, I'm not happy with that piece of code. I'm an advocate of Programming for People. But a person reading that code sample would have to do more mental decomposition than necessary. Which I would find impolite. Here's what I mean by mental decomposition:

First we read "PlayerManager" (right, so I've got this player manager), then we read ".GetPlayer(1)" (right so we've got player 1 of the player manager). Then we test this for null with " != null". (Ok, test if the player manager's player 1 is null).
Once we've got past this line of code, it would be nice to assume we could throw away our mental stack, and start with a clean slate, if you will. So we've passed the test in the "if" statement. Then we're going to assign to our inv variable. We'll take the playerManager, we'll get player 1 of the player manager, then we'll take the inventory of player 1 of the player manager. Ouch - my head hurts.

I would nearly always write that previous code sample as the following:

Player player = PlayerManager.GetPlayer(1);

if (player != null)
   inv = player.GetInventory();

Now, when I read that code, I see that Player 1 of the player manager is being assigned to a local variable - so I know this player is of interest. So I'll keep this player in my mental stack. Then I'll test that "this player" is not null. Then if I pass the test, I'll get the inventory of "this player". And this time, I had less mental decomposition.

And if nothing else, we've given the code more room to breathe! The train wrecks of the first example are hard on the eyes - it's hard to see where one element ends and the next begins. With the rewrite, the test and assignments are in much smaller and easier-to-digest chunks.

Rewriting the entire example, I would almost always write out the original train wreck as follows:

Ammo GetCurrentAmmo()
{
   Player player = PlayerManager.GetPlayer(1);

   if (player == null)
      return null;

   Inventory inv = player.GetInventory();

   if (inv == null)
      return null;

   Weapon[] weapons = inv.GetWeapons();

   if (weapons.Length == 0)
      return null;

   Weapon currentWeapon = weapons[0];

   if (currentWeapon == null)
      return null;
  
   Ammo currentAmmo = currentWeapon.GetAmmo();
   return currentAmmo;
}

The Law of Demeter says that you're only allowed to talk to your immediate neighbours. Strictly in the example above, I think I am allowed to ask the PlayerManager to do something, but I'm not allowed to ask the player to do something. Or the inventory. Or the weapons. And so on. But there are always exceptions to rules.

In this case, I think that if the GetCurrentAmmo function is in a class that made the most sense to know about getting the current player's ammo, then I'm fine with the multiple levels of indirection here. Otherwise each of the intermediate classes between this class and the Weapon class would need a "GetCurrentAmmo" forwarding function, which I feel would be a lot of extra code for little gain.

One last note on the code example - I've stored the result of the final "GetAmmo" function call before returning it. I'll explain why next time :)

Whenever I see the same "dot" phrase being used more than once in a function, I cringe. If the dot phrase happens to be a "double dot" phrase, I'll cringe even more. If a phrase repeats when it has more than 2 dots in it then I'll be sad that the person who wrote that code wasn't Programming for People.