The Case of the Caffeine Overdose: A CQS Mystery

The following is a dramatic re-enactment of an actual Command-Query Separation crime. The real entities have been replaced to protect their identities and to keep yours truly from getting pelted with a barrage of nerf bullets.

The Scene of the Crime:

This crime occurred within this seemingly innocuous piece of code that was designed to help developers cope with those nasty meetings that pointy haired bosses are always making them attend.

   1: while(meeting.InProgress)
   2: {
   3:     if (developer.IsSleepy() && meeting.IsBoring)
   4:     {
   5:         developer.HideBehindOverweightCoWorker();
   6:         developer.Doodle();
   7:     }
   8: }

About 100 iterations into the while loop, the developer entity shown here threw a null reference exception and went to that garbage collector in the sky.

Although everyone initially assumed that this was yet another meeting-induced suicide, the stack trace autopsy clearly showed that the object had mysteriously been set with lethal levels of caffeine.

Clues:

Forensic debuggers made the crucial breakthrough in the case after uncovering the following suspicious code snippet while running a routine ReSharper “Find Usages” command.

   1:
   2: if (task.IsImportant)
   3: {
   4:     while (developer.IsSleepy())
   5:     {
   6:         developer.RedoCriticalTask();
   7:     }
   8: }

After intense questioning, the suspected block of code finally admitted to never having experienced an infinite loop despite having no obvious exit conditions.

This was enough for investigators to attain a warrant for the source code of the IsSleepy method, which revealed the following damning evidence.

   1: public bool IsSleepy()
   2: {
   3:     bool isSleepyResult = hoursOfSleepLastNight < 4 && todaysMeetings.Length > 2 && isCloudy == true;
   4:     if (isSleepyResult)
   5:         AddEspressoShotToIV();
   6:     return isSleepyResult;
   7: }

The poor, unsuspecting developer object was injected with a shot of espresso each time the IsSleepy check was performed.

The IsSleepy method was clearly misleading clients by returning a value and thus impersonating a query method even though it surreptitiously performed an action (a.k.a. command) behind the scenes.

This is reckless violation of the classic Command-Query Separation law established by Bertrand Meyer (not to be confused with the more recent architectural level rule introduced by Eric Evans).

The unsuspecting victim didn’t have a chance.

The Sentence:

The person responsible for this atrocity was easily apprehended with a TortoiseSVN Blame and eventually sentenced to refactor the code. The reformed code can be seen here:

   1: public bool IsSleepy
   2: {
   3:     get
   4:     {
   5:         return hoursOfSleepLastNight < 4 && todaysMeetings.Length > 2 && isCloudy == true;
   6:     }
   7: }
   1: if (task.IsImportant)
   2: {
   3:     while(developer.IsSleepy)
   4:     {
   5:         AddEspresoShotToIV();
   6:         RedoCriticalTask();
   7:     }
   8: }

The Moral of the Story:

If your method returns a value, then you should avoid performing any actions.

There are a few rare exceptions to this design rule (e.g. Stack.Pop), but this is a firmly rooted expectation on the part of API consumers and violating it will inevitably lead to frustrating Heisenbugs.

More importantly, you should never ever ever write a blog post after watching a classic episode of CSI. No good can come of it.

8 Comments

  1. JonKristian March 26, 2010 4:04 am 

    Awesome post! Very funny and informative at the same time.

    Thanks!

  2. Giorgio Sironi March 28, 2010 4:10 pm 

    In the second code snippet, how do you come to suspect that isSleepy() is broken? I would have checked RedoCriticalTask() instead.

  3. Amy April 23, 2010 12:17 am 

    In the second code snippet, how do you come to suspect that isSleepy() is broken? I would have checked RedoCriticalTask() instead.

  4. Russell Ball April 23, 2010 11:57 am 

    @Giorgio & @Amy – The original smell was that I just didn’t notice an obvious place that caused the loop condition to exit. Someone might have easily put the exit condition in RedoCriticalTask() (although that also would have been bad design), but once you looked in there and didn’t see an exit condition, then you would have been compelled to go back to isSleepy().

Leave a Reply