Friday, August 22, 2014

Avoid Chained If ... else if ... Constructs, Part 1

  • Conditionals testing different dimensions should have different indentations

There are a number of reasons to avoid using chained "if ... else if ... else" constructs.  First among these is the ease with which they hide bugs.

Chained if-else constructs can hide bugs because each conditional can test a different dimension of the state space.  When this is done, successive "if" clauses explore successively smaller half-spaces in the state space.  As a result, much of the matrix of possibilities in the state space is left unexplored.

What does all of that mean?  We need an example.
if (A) doA();
else if (B) do_AB();
else if (C) do_A_BC();
else do_A_B_C();
Here, I use an underscore to indicate "not", so do_AB() represents "do something when A is false and B is true".

If we have three predicates (A, B and C) and they are orthogonal (independent), then there are 8 possibilities in the full matrix of possibilities:
_A_B_C
_A_BC
_AB_C
_ABC
A_B_C
A_BC
AB_C
ABC
yet, we have enumerated only four of these.  The source code using a chained if-else construct looks neat, but it has obscured the fact that all possibilities have not been canonically examined.  Time after time, I have exposed (and corrected) long-standing bugs by merely expanding a chained if-else construct to its canonical form, and filling in the missing cases.

The simple rule-of-thumb that may be followed to achieve this is that the conditionals in a chained if-else construct must all lie along the same dimension.  For example, if we have a straight enumeration:
enum signal_color
{
  SIGNAL_NONE,
  SIGNAL_RED,
  SIGNAL_YELLOW,
  SIGNAL_GREEN
};
then the following chained if-else would be permissible:
signal_color sc = ... ;
if (sc == SIGNAL_NONE) printf("Stop, then go.\n");
else if (sc == SIGNAL_RED) printf("Stop.\n");
else if (sc == SIGNAL_YELLOW) printf("Floor it!\n");
else if (sc == SIGNAL_GREEN) printf("Go.\n");
else error("Unexpected signal_color.\n");
Note that every clause tests the same variable, sc, and that every test uses equality.  (Bit tests are different, as shown below.)  Since all the tests lie along the same dimension, it makes sense to add an else clause, to capture unhandled cases.  This is good Software Engineering practice, as it guards against unexpected behavior in this code, should another element be added to the enumeration in the future.

However, if we have a bit-mapped enumeration:
enum signal_color_components
{
  SIGNAL_COMPONENT_BLUE = 1<<0,
  SIGNAL_COMPONENT_GREEN = 1<<1,
  SIGNAL_COMPONENT_RED = 1<<2,
};
Then the following chained if-else is not permissible:
signal_color_components scc = ... ;
if (scc & SIGNAL_COMPONENT_RED) printf("Stop.\n");
else if (scc & SIGNAL_COMPONENT_GREEN) printf("Go.\n");
else printf("Stop, then go.\n");
In this version, it is evident that we left room for a bug, and the damned thing crawled right in!  We want the case scc & SIGNAL_COMPONENT_RED && scc & SIGNAL_COMPONENT_GREEN to print out "Floor it!\n", and yet this is not done.

The predicates testing each bit of a bitmap are orthogonal predicates (i.e. predicates testing different dimensions), so we fell right into the trap.  If the rule of thumb is followed, we would first rewrite the above as:
signal_color_components scc = ... ;
if (scc & SIGNAL_COMPONENT_RED)
{
  printf("Stop.\n");
}
else
{
  if (scc & SIGNAL_COMPONENT_GREEN) printf("Go.\n");
  else printf("Stop, then go\n");
}
Now it is obvious that the first clause (where the signal color has a red component) is missing a case. After adding back the missing case:
signal_color_components scc = ... ;
if (scc & SIGNAL_COMPONENT_RED)
{
  if (scc & SIGNAL_COMPONENT_GREEN) printf("Floor it!\n");
  else printf("Stop.\n");
}
else
{
  if (scc & SIGNAL_COMPONENT_GREEN) printf("Go.\n");
  else printf("Stop, then go\n");
the code segment will behave as intended.  Goodbye bug!

No comments:

Post a Comment