GT!Blog

One of the topics that people like to talk about is technical debt. However, when you actually get into what creates this technical debt, you find a lot of simple repeating patterns that reduce code readability. So, I decided to run through a pile of them to see if there are any simple fixed. The first one I want to look at is the use of boolean types. Booleans are an extremely useful construct in programming, but they are also a source of hard to catch errors. Here I just want to look at the problems that bools can create, and also propose a set of guidelines for reducing the perceived technical debt they create.

The Bool Asymmetry

One of the main problems with boolean types is the asymmetry between writing and reading code. Writing code using bools is extremely easy. Reading and modifying existing code that uses bools, on the other hand, is where the trouble begins. Let’s take a quick example. Suppose we have some kind of function that parses text data from a buffer:

void ProcessBuffer(void *buffer)

But later we need to add support for Unicode. So we modify our function to handle the Unicode. If we were Windows programmers this would mean UTF16. The easiest way to go is to add a bool with a default parameter. This way, we don’t need to change any existing code.

void ProcessBuffer(void *buffer, bool is_unicode = false)

But then we need to add support for UTF8 because our code is starting to get UTF8 input in some file formats:

void ProcessBuffer(void *buffer, bool is_unicode = false, bool is_utf8 = false)

Then the situation arises where we need to process big endian UTF16 as well as little endian:

void ProcessBuffer(void *buffer, bool is_unicode = false, bool is_utf8 = false, 
                   bool is_utf16be = false)

And later the function is called in cases where we have to deal with Unicode normalization, and even later we discover that we need to be able to handle two different normalization forms, giving us:

void ProcessBuffer(void *buffer, bool is_unicode = false, bool is_utf8 = false,
                   bool is_utf16be = false, bool normalize = false,
                   bool normalize_D = false)

The above sequence of events is not particularly far fetched. I could almost guarantee that every experienced programmer has come across a situation where adding a bool to a function is the easiest solution. So what’s the problem? Well, when we are writing code that calls this function, it’s not too big a deal. Our IDE will pop up the parameter list when we are writing code to call the function, and as long as we have named the parameters appropriately, it should be pretty easy to get the value right. However, what happens if we come along at a later stage and need to read this code? Well, then we are faced with something like this:

// Process the buffer as utf-8
ProcessBuffer(buffer, true, false, false, true, false);

Can you spot the bug in this code? The problems for a person reading the code should be abundantly clear. There are quite a few problems here, and no amount of static analysis is going to help us out. Probably the worst problem with this code is the misleading comment. Rather than helping us understand the code, it actually gives us false confidence about the meaning of the boolean value.

Enums FTW

A better option in terms of readability is using an enum. If we had started off the same example except using enums, we would probably end up with something like the following sequence of changes:

enum {
    eLegacyEncoding = 1,
    eUnicode = 2
} encoding_t;
void ProcessBuffer(void *buffer, encoding_t encoding = eLegacyEncoding)

enum {
    eLegacyEncoding = 1,
    eUnicode = 2, // Keep for backwards compatibility or refactor out
    eUTF16 = 2,
    eUTF8 = 3
} encoding_t;
void ProcessBuffer(void *buffer, encoding_t encoding = eLegacyEncoding)

enum {
    eLegacyEncoding = 1,
    eUnicode = 2, // Keep for backwards compatibility or refactor out
    eUTF16 = 2,   // Keep for backwards compatibility or refactor out
    eUTF16LE = 2,
    eUTF8 = 3,
    eUTF16BE = 4,
} encoding_t;
void ProcessBuffer(void *buffer, encoding_t encoding = eLegacyEncoding)

enum {
    eLegacyEncoding = 1,
    eUnicode = 2, // Keep for backwards compatibility or refactor out
    eUTF16 = 2,   // Keep for backwards compatibility or refactor out
    eUTF16LE = 2,
    eUTF8 = 3,
    eUTF16BE = 4,
} encoding_t;
enum {
    eDoNotNormalize = 1,
    eNormalizeC = 2,
    eNormalizeD = 3
} normalization_t;
void ProcessBuffer(void *buffer, encoding_t encoding = eLegacyEncoding,
                   normalization_t norm = eDoNotNormalize)

And now, spotting the bug is so much easier:

// Process the buffer as utf-8
ProcessBuffer(buffer, eUTF16, eNormalizeC);

In fact, the readability is improved so much that the preceding comment becomes superfluous.

Return Values

Above we saw the problem as applicable to function arguments. For return values, things are a little bit different. For functions that are boolean in nature, using a bool return type is fine. For example:

bool IsQueueEmpty()

When viewed in context, readability is preserved

// Close once the queue is empty:
if (IsQueueEmpty()) {
    Close();
}

Where problems tend to arise is when a function needs to be modified to return just a little bit extra information. The temptation to simply change a void return type into bool is strong, but the cost is reduced readability. Take a look at the following prototype. What do you think the return value represents?

bool GetMessage(msg_t *message)

The answer is that everyone will interpret this differently. For me, I would assume that a return value of false indicates no message to get, while true would indicate a message was returned. However, it could just as easily be that a return value of false means error and true means no error (but not necessarily that a message was returned). And if you’re a Win32 programmer, you might guess that a return value of false indicates that a quit message was received, whereas true indicates either a non-quit message or an error. (This is how the Win32 GetMessage function actually works.) It could mean anything. Unfortunately, when we’re writing a function like this, the intended meaning seems clear because the problem we are trying to solve is fresh in our minds. It’s only after you have to come back to some code that you haven’t touched for a while that the opacity of the function meaning becomes clear.

Why We Don’t

One of the reasons people opt for the boolean over the enum or named constant route is that it is just so easy. Writing enums involves added boilerplate, and fills up your namespace with types that are only applicable in very specific contexts. Your function prototype/interface changes from a single line to multiple lines of code. Nevertheless, if you are interested in the future readability of your code, I think the tradeoffs are worth it.

The Guidelines

After hitting the same problems of readability in my own code many, many times, I now have a set of guidelines I try to follow. The increased readability of old code really makes it worthwhile. The basic rule:

• All boolean function arguments and return values should be replaced with enums.

However, the following important exceptions apply:

• Function args for setter functions for boolean values.
• Function args for enable/disable style functions. E.g. SetEnabled(bool) and SetVisible(bool) are acceptable.
• Return values for getter functions for boolean values.
• Return values for functions that explicitly test for a true/false condition.

Examples in the Wild

To close with, I want to share an example of a bug that a co-worker recently found in an open source project. Can you spot the error? Without access to the function prototypes, the bug is completely invisible.

if (mysql->options.connect_timeout >= 0 &&
    vio_wait_or_timeout(net->vio, FALSE, mysql->options.connect_timeout * 1000) < 1)
{
  my_set_error(mysql, CR_SERVER_LOST, SQLSTATE_UNKNOWN,
               ER(CR_SERVER_LOST_EXTENDED),
               "handshake: waiting for inital communication packet",
               errno);
  goto error;
}
if ((pkt_length=net_safe_read(mysql)) == packet_error)
{
  if (mysql->net.last_errno == CR_SERVER_LOST)
     my_set_error(mysql, CR_SERVER_LOST, SQLSTATE_UNKNOWN,
               ER(CR_SERVER_LOST_EXTENDED),
               "handshake: reading inital communication packet",
               errno);
  goto error;
}

Now suppose that we replace the boolean FALSE on the second line with a descriptive enum label such as IO_WRITE. See the bug now?