Branching on multiple conditions made readable

If you need to branch on a set of conditions, if..then..else trees quickly begin to look real ugly. On top of that, they become nests of hard to find bugs and a nightmare for the maintenance programmer. A good example of this tragedy in action can be seen in Listing 1. I didn’t write this myself, but I have written any number of code sequences just as bad or worse, so I’m certainly no better than the author of this example. But looking at my own code, I don’t notice how hard it is to read. Don’t worry about the function calls to Trace() and Notify() or any of the others. I left them in so the code would look like code you encounter in real life, not because they have any particular significance to the topic of the article. Also note that in this case, more comments would almost certainly not help. Either they wouldn’t clarify much, or you wouldn’t be able to trust them anyway. The code is in Delphi, but I’m sure it wouldn’t be any easier to read if it had been in any other language.

while True do begin

  bResult:= GetQueuedCompletionStatus(
    fhCompletionPort,ulTransf,Cardinal(pPH),POverlapped(pPIO),INFINITE);

  if (NOT bResult) and (pPIO=nil) then begin
  { function does not dequeue a completion packet }
    Trace('NO packet, Win32 Error Code: '+ IntToStr(GetLastError));
  end

  else if (NOT bResult) and (pPIO<>nil) then begin
  { function dequeues a completion packet for a failed I/O operation }
    Trace('IO failed packet, Win32 Error Code: '+ IntToStr(GetLastError));

  { Expect this for a socket that was killed before disconnecting }
    Notify(kNFY_DISCONNECT,pPH^.socket);
    DestroyPerIoData(pPIO);
    closesocket(pPH^.socket);
    FreeMem(pPH, SizeOf(TPerHandleData) );
  end

  else if (ulTransf=0) then begin
  { EOF : connection closed }
    Notify(kNFY_DISCONNECT,pPH^.socket);
    DestroyPerIoData(pPIO);
    closesocket(pPH^.socket);
    FreeMem(pPH, SizeOf(TPerHandleData) );
  end

  else begin
  { function dequeues a completion packet for a successful I/O operation! }

    if (pPIO^.ioType=opInput) then begin
      Trace(lcRecv,'socket['+ IntToStr(pPH^.socket) +'] Recv '
        + IntToStr(ulTransf));
      bResult:= ReadIoDataToMsg(pPH^.acMsg, pPIO^.ioBuf.buf, ulTransf, sMsg);
      DestroyPerIoData(pPIO);
      IssueRead(pPH,CreatePerIoData);
    end

    else if (pPIO^.ioType=opOutput) then begin
      Trace(lcSend,'socket['+ IntToStr(pPH^.socket) +'] Sent '
        + IntToStr(ulTransf) +' ['+ PChar(pPIO^.ioBuf.buf) +']');
      DestroyPerIoData(pPIO);
    end;
  end;
end;

Listing 1

This program code is impossible to follow or to verify; at least it is to me. I just sit there mumbling: “if bResult is True, but bytes transferred is zero, I end up at… ah… right, but if it’s False and pPIO is nil and … no… I mean not nil and transferred bytes is… eh…”.

The root cause of this particularly evil example is that the GetQueuedCompletionStatus() Win32 API function has a really horrible interface. Just looking at the documentation of the return results, is enough to floor anyone:

Return Values

If the function dequeues a completion packet for a successful I/O operation from the completion port, the return value is nonzero. The function stores information in the variables pointed to by the lpNumberOfBytesTransferred, lpCompletionKey, and lpOverlapped parameters.

If *lpOverlapped is NULL and the function does not dequeue a completion packet from the completion port, the return value is zero. The function does not store information in the variables pointed to by the lpNumberOfBytesTransferred and lpCompletionKey parameters. To get extended error information, call GetLastError. If the function did not dequeue a completion packet because the wait timed out, the error returned is WAIT_TIMEOUT.

If *lpOverlapped is not NULL and the function dequeues a completion packet for a failed I/O operation from the completion port, the return value is zero. The function stores information in the variables pointed to by lpNumberOfBytesTransferred, lpCompletionKey, and lpOverlapped. To get extended error information, call GetLastError.

What the text boils down to is that three different and almost independent returned values determine what actually happened in the call. The three return values are (using the naming of the parameters as in the Delphi example code):

• bResult: may be True or False
• pPIO: may be nil or assigned a pointer value
• ulTransf: may be zero or hold the number of bytes transferred

Theoretically, these three variables can determine any one of eight possible states. Two of these states indicate that no packet was dequeued, two indicate that the I/O failed, one indicates a closed connection and one indicates a successful I/O. The remaining two states should never occur. In the code in Listing 1, it’s very difficult to see if all the eight states have been covered and if all the code is reachable. I really had to find a better design to make it more readable.

Nested branches

My first thought was to test only one condition per if/then/else, then have another two layers below each one:

if bResult then begin
  if assigned(pPIO) then begin
    if ulTransf > 0 then begin
      …success I/O…
    end
    else begin
      …connection closed…
    end
  end
  else begin
    …illegal case…
  end
end
else begin
  if assigned(pPIO) then begin
    …failed I/O…
  end
  else begin
    …no dequeue…
  end
end

This method is even worse and will often cause a lot of code duplication. Just looking at it in outline makes me go all cross-eyed. The code example in Listing 1 is actually, believe it or not, the most readable way to use if/then/else constructs for branching on multiple conditions.

Let’s forget about if/then/else then

To get a grip on this situation, I’ll take a totally different approach. I start out with drawing up a decision table in a comment block to make sure I understand what is supposed to happen and to ensure that the future reader of the code also gets the idea (Listing 2).

{ a little helpful decision table for use in the case..of
                 +---------------------------+---------------------------+
                 |        pPIO = nil         |      pPIO <> nil          |
                 |-------------+-------------|-------------+-------------|
                 | ulTransf = 0| ulTransf > 0| ulTransf = 0| ulTransf > 0|
 ----------------|-------------|-------------|-------------|-------------|
 bResult = False | No Dequeue  | No Dequeue  | Failed I/O  | Failed I/O  |
 ----------------|-------------|-------------|-------------|-------------|
 bResult = True  | Illegal     | Illegal     | Conn Closed | Success I/O |
 ----------------+-------------+-------------+-------------+-------------+
}

Listing 2

Calling the GetQueuedCompletionStatus() results in three Boolean conditions, so I need a three-dimensional table. Since that is real hard to do in a source file unless you’ve got holographic displays, I double the columns for transferred bytes instead.

After I get the table just so, I translate it into a three-dimensional table in Delphi. If I have two conditions, it will be a two-dimensional table and if I’ve got four, it’ll be a four-dimensional table, of course. Each of the dimensions has only two values, False and True (in that order). In most other languages, I would have to use numeric indexes instead. The value type of the table is an enumeration corresponding to the contents in the table. Since this particular table has five different values, I create an enumeration with expressively named constants (Listing 3).

type
  eCompRes = (eNoDequeue, eFailedIO, eIllegal, eConnClosed, eSuccessIO);

Listing 3

Now I can create and initialize the decision table in code. If I define the table as a constant, it gets compiled into the executable and takes zero runtime (Listing 4).

const
  aECR : array[False..True, False..True, False..True] of eCompRes = (
                ((eNoDequeue,   eNoDequeue ), (eFailedIO,   eFailedIO)),
                ((eIllegal,     eIllegal),    (eConnClosed, eSuccessIO))
  );

Listing 4

Now I put this declaration into my source file directly below the decision table, so that the reader can easily verify the correspondence between the two, making his or her eyes bounce up and down, as if watching a vertical game of tennis (Listing 5).

const
{ a little helpful decision table for use in the case..of
                 +---------------------------+---------------------------+
                 |        pPIO = nil         |      pPIO <> nil          |
                 |-------------+-------------|-------------+-------------|
                 | ulTransf = 0| ulTransf > 0| ulTransf = 0| ulTransf > 0|
 ----------------|-------------|-------------|-------------|-------------|
 bResult = False | No Dequeue  | No Dequeue  | Failed I/O  | Failed I/O  |
 ----------------|-------------|-------------|-------------|-------------|
 bResult = True  | Illegal     | Illegal     | Conn Closed | Success I/O |
 ----------------+-------------+-------------+-------------+-------------+
}

  // translated into reality, this becomes:
  //     array[--bResult--, ---pPIO----, -ulTransf--] of ...
  aECR : array[False..True, False..True, False..True] of eCompRes = (
                ((eNoDequeue,   eNoDequeue ), (eFailedIO,   eFailedIO)),
                ((eIllegal,     eIllegal),    (eConnClosed, eSuccessIO))
  );

Listing 5

Once I have the decision table, I can evaluate the three conditions, look up the enumeration result and branch, all in one fell swoop. As you can see, it becomes very easy to have a branch for the illegal (and unexpected) combinations as well (the eIllegal case). See Listing 6.

while True do begin

  bResult:= GetQueuedCompletionStatus(
    fhCompletionPort, ulTransf, Cardinal(pPH), POverlapped(pPIO), INFINITE);

  // evaluate, lookup and branch
  case aECR[bResult, assigned(pPIO), ulTransf > 0] of

  eNoDequeue:
    begin
      Trace('NO packet, Win32 Error Code: '+ IntToStr(GetLastError));
    end;

  eFailedIO:
    begin
      Trace('IO failed packet, Win32 Error Code: '+ IntToStr(GetLastError));
    { Expect this for a socket that was killed before disconnecting
      Not freeing a IdTCPClient before killing process will do it.  }
      Notify(kNFY_DISCONNECT, pPH^.socket);
      DestroyPerIoData(pPIO);
      closesocket(pPH^.socket);
      FreeMem(pPH, SizeOf(TPerHandleData));
    end;

  eIllegal:
    // this should never happen
    assert(False, ‘Illegal results from GetQueuedCompletionStatus’);

  eConnClosed:
    begin
      Notify(kNFY_DISCONNECT, pPH^.socket);
      DestroyPerIoData(pPIO);
      closesocket(pPH^.socket);
      FreeMem(pPH, SizeOf(TPerHandleData));
    end;

  eSuccessIO:
    begin
      case pPIO^.ioType of

      opInput :
        begin
          Trace(lcRecv, 'socket['+ IntToStr(pPH^.socket) +'] Recv '+
            IntToStr(ulTransf));
          bResult := ReadIoDataToMsg(pPH^.acMsg, pPIO^.ioBuf.buf,
            ulTransf, sMsg);
          DestroyPerIoData(pPIO);
          if (bResult) then begin
            Notify(kNFY_PING);
            // TODO: do something with the string HERE!
          end;
          IssueRead(pPH, CreatePerIoData);
        end;

      opOutput:
        begin
          Trace(lcSend, 'socket['+ IntToStr(pPH^.socket) + '] Sent '
            + IntToStr(ulTransf) + ' [' + PChar(pPIO^.ioBuf.buf) + ']');
          DestroyPerIoData(pPIO);
        end;

      else
        Trace('*** Illegal pPIO.ioType');
      end; // case pPIO^.ioType
    end; // eSuccessIO
  end; // case aECR[...]
end; // while True

Listing 6

GoodNess abounds

Now, this, dear reader, gives the maintenance programmer a fighting chance to see what I’m actually doing. It also forces me to cover all cases explicitly, but without duplicating code. And, on top of that, it probably executes faster. It simply overflows with GoodNess®. There’s just one possible runtime disadvantage and that is that all conditions used to index into the array are always evaluated. If that is unacceptable, I divide the case tree into nested trees, each based on its own decision table. (As an illustration, the branching on pPIO^.ioType under the eSuccessIO case is nested which doesn’t really make it any more difficult to follow the processing flow. The reason it is nested is that it can’t be evaluated at all unless pPIO is assigned a value.)

The principle of multiple branch conditions should be applicable to any language that allows multidimensional arrays, enumerations and switch statements. Delphi is particularly elegant in that it allows me to index arrays using Booleans, but C++ or C# would need only slightly more code to achieve the same effect, while maintaining the same level of readability.