One of the most difficult parts of writing really good code is coming up with names for things.

A good name is a tricky deal. It shouldn't be too short, or too long, or too specific, or too vague, or too likely to become incorrect due to changes in implementation detail, and on and on and on. Goldilocks and the Three Bears? Yeah, that girl is downright tolerant compared to the rigors of picking good names in a program. You want to talk about things needing to be Just Right...


Suppose you're looking for a bug where a program appears to run successfully, but has actually failed. By random chance, you read across this code snippet:

FooAPIResult result = FooAPIDoComplicatedStuff();
if(FooAPIResultIsError(result))
{
DoFailure();
return;
}

// continue with complicated stuff

How many times would you read over that code without seeing the bug?

"What bug?" you ask! Surely that's a pretty simple thing, there, and there can't be a bug in something so clear... right? I mean, the API names are right there! It's good and self-documenting and trivial and there's no possible way it could be wrong! (This blindness, by the way, is especially powerful when you're reading your own code. In this case, it wasn't my own code, but I still read past that section dozens of times before catching the mistake.)


So let's take a moment and think about what this API call sequence is meant to do. First, we DoComplicatedStuff() and store the result of the process in a Result variable, which is presumably an enum or some other set of constants defined by the API. We're given a FooAPIResult typedef which tells us that we can ask the Foo API for details about what a given Result means, but we shouldn't assume anything - the values might change between versions, for instance.

So far so good.

Now, we want to ask the API if the process was successful. We look at the API documentation, and behold, there are two functions:

bool FooAPIResultIsCompletion(FooAPIResult result);
bool FooAPIResultIsError(FooAPIResult result);

Well, let's see. In our code, we aren't done with the complicated process, so clearly we don't want to check for completion. We do, however, want to early-out if an error occurs, so obviously the function we want is the second one: FooAPIResultIsError(). And sure enough, our code uses that function.

But you know... just in case... let's check for completion instead. Turns out, the bug is still there!


If you aren't seeing the bug yet, don't feel bad. It took me an hour of staring at those few lines in bewilderment to figure out what was going on, and plenty of other people have studied the code and missed it as well. I almost feel like I was lucky in finding it at all, given how devious this bug is.


Spoiler time! The bug is that the API has lied to you.

FooAPIResult is opaque to us outside the API; we're not supposed to know or care what the numbers mean. That's why the API lets us ask it what the numbers mean. But the API doesn't paint an accurate picture.

Suppose I tell you that FooAPIResult is internally an enum that looks like this:

enum FooAPIResultReal
{
Result_NotStarted,
Result_Started,
Result_Step_One,
Result_Step_Two,
Result_Step_Three,
Result_Completion,
Result_Error_Something_Barfed,
Result_Error_Generic_Failure,
Result_Error_Filler
};

Hopefully at this point you can see where I'm going with this, but just in case you're like me and would rather read the book to find out the answer to the mystery than try to solve it on your own, here's the answer:

bool FooAPIResultIsCompletion(FooAPIResult result)
{
return (static_cast(result) == Result_Completion);
}

bool FooAPIResultIsError(FooAPIResult result)
{
FooAPIResultReal real = static_cast(result);
if(real == Result_Error_Something_Barfed || real == Result_Error_Generic_Failure || real == Result_Error_Filler)
return true;

return false;
}


What's happened is that the API has omitted important facts. Presumably, this was done in the interests of keeping implementation detail hidden from the API consumer, which is typically a noble goal. In this case, though, it has backfired.

The real situation is that there are more than just two types of Result code: there is more going on than just "I'm done" or "I barfed." There's a whole class of Result states that could be returned by the API that aren't handled by the lookup functions at all.

Going back to our original problematic code:


FooAPIResult result = FooAPIDoComplicatedStuff();
if(FooAPIResultIsError(result))
{
DoFailure();
return;
}

// continue with complicated stuff

Suppose result is Result_Step_Two. We check to see if this is an error, which of course (to the API designer's mind) it is not. Except it's also not completion, which means that even when we change the code to check for completion instead, we won't trap the problem. In this case, the problem is that Step_Two indicates that the process died halfway through, and we shouldn't trust its results.

But the API has deceived us. We trusted it, because it was a nicely designed opaque API that hid implementation detail from us, just as a proper API should. And that trust caused a bug that has lurked for untold aeons in some very old and well-tested code, because the circumstances leading to its triggering are painfully hard to reproduce. Thus we have the perfect storm: the bug exists, but it's hard to make it show up, and no matter how many times you read the code, it looks like it's obeying the API contract.

It wasn't until I yanked open the lid of the API and looked at the guts that I figured it out. By pretending to be hiding implementation detail, the API screwed its customers, because in reality, the implementation detail could leak in ways that were extremely hard to catch.




The moral of the story? Naming is hard. And when you pick names for something, try your best to make sure you aren't lying to the guy who has to consume your API.