Jump to content

  • Log In with Google      Sign In   
  • Create Account

FREE SOFTWARE GIVEAWAY

We have 4 x Pro Licences (valued at $59 each) for 2d modular animation software Spriter to give away in this Thursday's GDNet Direct email newsletter.


Read more in this forum topic or make sure you're signed up (from the right-hand sidebar on the homepage) and read Thursday's newsletter to get in the running!






Lies, damned lies, and API function names

Posted by ApochPiQ, 31 October 2011 · 596 views

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<FooAPIResultReal>(result) == Result_Completion);
}

bool FooAPIResultIsError(FooAPIResult result)
{
	FooAPIResultReal real = static_cast<FooAPIResultReal>(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.




Nice story!

I find functions easy to name, personally. If they are hard to name, Code Complete taught me that it's probably because it's trying to do too much at once.

I find classes really hard to name, though. Probably because my classes do too much. Posted Image
Now I want to rewrite my entire code base... good read apoch. Just a question out of curiosity.. is it common in the industry to have a class with functions, or just a group of functions that are basically just 4 lines of code? To keep the functions doing one thing only.

I've been re looking over alot of my old code that I use alot, and going by what I've gleamed from the masses here. Alot of the code should be split up into more specific functions/objects. Sometimes it makes sense, but for me it always feels like code bloat if I split them into multiple functions/objects.
A lot of the game industry still uses C-style APIs for things, especially for long-lived libraries; it's not uncommon to see a blend of C++ style APIs and C-style depending on the relative ages of the various bits of code.

As for splitting code up into multiple objects/functions: just remember, there is no tax on writing a new function. If it makes your code cleaner or easier to understand or easier to reuse, then there is no reason not to split it up.
Apoch, your post puts me in mind of certain windows APIs.... Posted Image I've felt more like I'm reverse engineering or black-box testing than coding with some of them. Bugs 'suddenly' appearing in code which has apparently been shipping for the last ten years, and which turns out to have always been wrong once you work out what msdn was actually trying to say (or not).

I actually like fairly long names, as long as they're appropriate and descriptive. It reduces the temptation for colleagues to add things that at a glance seem to fit, but which muddy or break the design. I've been guilty of the same, and sometimes a longer more specific name would have made me think twice about adding stuff.

is it common in the industry to have a class with functions, or just a group of functions that are basically just 4 lines of code? To keep the functions doing one thing only.


Assuming C++, here's a related quote from Stroustrup that really affected my thinking on design :

My rule of thumb is that you should have a real class with an interface and a hidden representation if and only if you can consider an invariant for the class.


It took me years to really appreciate the depth of this insight. Free functions are incredibly valuable, as they have the minimum amount of coupling. They're the most reuseable thing in the toolbox. Classes are incredibly valuable as a way to enforce invariants over a group of functions and data. Without invariants to enforce, grouping functions in a class just makes the functions slightly more difficult to reuse. Posted Image

December 2014 »

S M T W T F S
 123456
78910111213
14151617181920
21222324 25 2627
28293031   

Recent Entries

Recent Comments

PARTNERS