Concurrency and the fallacy of self-documenting code

There seems to be a ongoing debate about whether comments are necessary or even good in your code. The self-documenting code camp claims that if your code is well-factored and well-written then there is no need for comments. Any talented programmer will be able to follow the intent of the code and understand it.

It seems to me that there are two problems with this:

  • Mismatch between coding paradigm and problem domain.
  • Concurrency.

Let me give you an example of the second one. I was recently debugging some open source telephony code for a client of mine when I ran across this pseudo-code used to make a call:

this.setCallID(reserveCallId());

provider.createCall(this.getCallID(), address, terminal, dialedDigits);

this.toActive(Event.CAUSE_NEW_CALL);

I was getting a null-pointer exception in the last line, which I traced down to the “call id” being null. But “reserveCallId()” is guaranteed never to return null.

After uncovering the cause, the psudo-code fix was:

this.setCallID(reserveCallId());

provider.createCall(this.getCallID(), address, terminal, dialedDigits);

if(this.getCallID() == null) {  return null;}

this.toActive(Event.CAUSE_NEW_CALL);

At first glance, this new third line makes no sense. Another programmer would not easily understand why we would stop what we are doing if the call id is null at this point. Without comments, it is hard to see how any amount of refactoring or renaming of code would explain this logic.

The underlying reason for this odd code is that, depending on the telephony system, createCall() may not return until the call is connected. If, while being set up, the call is canceled  by another thread issuing a hangup command, then the call state is wiped out and the call id is set to null. Perhaps instead of checking for a null call id to see if this has happened, we can check the call state, but that is still not very clear: why would a newly created call be in an INVALID state?

A few lines of comments — ten seconds worth — is enough to explain it to whoever has to follow in your shoes:

// check if the call has been disconnected during set up,
// such as by an asynchronous hang-up
if(this.getCallID() == null) {  return null;}

Is that so hard?