Idle Time

I’m going to confess a realization that I’ve come to regarding one of my oldest and strongest API-designing habits—that I now believe that habit to be a mistake.

Whenever I design an API, my mind habitually wanders to how its functions should handle NULL for various arguments. Usually I make NULL do something special. In this way, I give NULL the implication of “do this operation with what you already have”, or “do this with the default value for this argument”.

This is a mistake. NULL should be an error.

There are two reasons why this is so:

1. Clarity

   Compare these two pieces of code*:

   remove_filter(NULL, NULL);

   remove_all_filters();

   Which is clearer? Which makes the statement’s action more explicit?

2. Robustness

   What happens when (not if) somebody passes NULL by accident?

   Consider this:

   hookName = [filterNameField stringValue];
   [self removeFilterForHook:hookName
   target:nil
   selector:NULL];

   What happens when you forget to connect the filterNameField outlet in Interface Builder, or when one of your localizers accidentally disconnects it, or when you or another programmer on the project deletes filterNameField, replaces it with something else, or cuts-and-pastes it (e.g. into a new tab view item), and fails to reconnect it?

   When the outlet is not connected, filterNameField will at runtime be nil. Messages to nil return nil, so hookName is assigned nil. Then you pass this value (nil) as the hook name to removeFilterForHook:target:selector:**. Oops—you just removed all the filters.

   The solution is to add a new method called removeAllFiltersForHook:, and make nil/NULL illegal in all four arguments (the one to removeAllFiltersForHook: and the three to removeFilterForHook:target:selector:). Thus:

   hookName = [filterNameField stringValue]; //stringValue returns nil, as before
   [self removeAllFiltersForHook:hookName]; //Assertion failure

   Rather than removing those filters and causing invalid (unfiltered) data to pass through, which will hopefully be noticed now but Murphy’s Law says will be noticed much later (after you have forgotten this code), the new code comes to a screeching halt with a message in the console. If your program is seriously robust, your crash-catcher (for you do have one) will catch the exception and present a crash-report dialog.

   And, of course, your code is much clearer—no longer must the maintenance programmer (you, in six months) think about and interpret what you meant by “target:nil selector:NULL”, as such magic has been banished.

Counterpoint, and the rebuttal to the counterpoint

The main reason why I’ve made functions accept NULL and handle it specially is so that I can implement some higher-level method (such as removeAllFiltersForHook:) with it. For example, a function in Growl’s Carbon API:

Growl_RegisterWithDictionary

Register your application with Growl without setting a delegate.

Boolean Growl_RegisterWithDictionary(CFDictionaryRef dict);

When you call this function with a dictionary, GrowlApplicationBridge registers your application using that dictionary. If you pass NULL, GrowlApplicationBridge will ask the delegate (if there is one) for a dictionary, and if that doesn’t work, it will look in your application’s bundle for an auto-discoverable plist.

<rant>

</rant>

The reason why I had this function act this way was so that Growl_Reregister could be a one-liner. Here’s the code (Growl BSD license):

void Growl_Reregister(void) {
Growl_RegisterWithDictionary(NULL);
}

Convenient. In turn, Growl_RegisterWithDictionary does some set-up, then passes the registration dictionary (whether it was passed in, obtained from delegate, or otherwise produced or assembled) off to another, internal function. I wouldn’t want to duplicate the code of Growl_RegisterWithDictionary, right?

But I don’t have to. All the set-up code from Growl_RegisterWithDictionary does is:

1. If a dictionary was passed in, make sure that that dictionary is completely filled-in (and if it isn’t, fill it in from the delegate’s dictionary). This is handled by another function—Growl_RegisterWithDictionary just calls that function.
2. If a dictionary was not passed in, get a filled-in dictionary from one or more other places.

If we make Growl_RegisterWithDictionary assert that its dictionary is not NULL, the code changes to:

1. Assert that a dictionary was passed in.
2. Make sure that the dictionary is completely filled-in.

Neither of these would be duplicated in Growl_Reregister—it takes no arguments, and the dictionary that it gets from elsewhere either is already completely filled-in by the time Growl_Reregister receives it or is not usable anyway. More to the point, any code that is using Growl_RegisterWithDictionary(NULL) would have to change to Growl_Reregister(), making it both clearer and shorter.

---

* The functions are named this way because I was thinking about WordPress at the time, but this really has nothing to do with WP, and I don’t know if it is even relevant at all. That is to say, I don’t know whether WP’s remove_filter treats null specially in any way. Case in point. ↶

** Alert readers will note the similarity to NSNotificationCenter’s API. This is not intentional, but the similarity is real, and you should take from it that this situation is not merely theoretical: it certainly has happened, and will happen again. ↶

Categories: Best practices; Programming. | Comments: 6 (feed).