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.

Popularity: 2% [?]