Concurrency and the fallacy of self-documenting code

Posted on by

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?

6 thoughts on “Concurrency and the fallacy of self-documenting code”

  1. Better still, the developer should have developed a checked exception that describes the error:

    if(this.getCallID() == null) {
    throw new CallDisconnectedException( this );
    }

    This catches the problem at compile-time.

  2. Well, true, except that this code was implementing a Java API (JTAPI), so we don’t control what exxceptions can be thrown. The Call.connect() method only allows six exceptions:
    ResourceUnavailableException
    PrivilegeViolationException
    InvalidPartyException
    InvalidArgumentException
    InvalidStateException
    MethodNotSupportedException

    I guess we could throw an InvalidStateException, but the call did go though and was in a valid state when you started, so this isn’t quite correct either. If you look at the API post-conditions for the call, you see that the JTAPI API doesn’t even really cover this possibility…

  3. A subclass of InvalidStateException that accurately describes the problem would work. The call state may have been fine at the beginning of the method, but is obviously invalid by the time the null value is supposed to be returned.

    At a higher level, returning null in an application, generally speaking, screams of a poorly thought-out interface. The null value is probably Java’s biggest mistake.

  4. But would anyone catch it and handle it properly? This is a JTAPI implementation that is going to be used by 3rd party applications.

    It’s an interesting idea. It doesn’t negate the fact that a simple comment when you are returning your null or throwing your exception will help future maintainers understand why this code path appeared in the first place.

    (Nice to hear from you after all this time Dave.)

  5. I saw an SX-200 today. Several of them, I think, piled on a dolly, on the way out of the office.

    The issue I have with comments is that I should not have to look at the source code to use an API — much less the internal comments — to see how the error conditions of a method are defined. As a developer using a third-party library there may be situations when the source code is unavailable.

    This means there are two issues.

    1. Documentation. There should be an API comment indicating that the method can also through a subclass of InvalidStateException (i.e., CallDisconnectedException).

    There should also be a class comment for CallDisconnectedException that states what the exception means. (It would be nice if the Javadocs could point you to a list of methods that throw the exception [as defined by the @throws markup], alas that list would likely be unwieldy [especially for Exception itself!].)

    2. The code returns null. It must throw an exception. A 3rd party will handle it about as well as handling a null return value. Returning null, however, offers the developers no information — well, certainly much less than throwing a suitably named (and well documented) exception.

    I’ve been swamped, Richard. Nice to see you are as well!

Leave a Reply

Your email address will not be published. Required fields are marked *