NULL as a function argument should be illegal

2007-02-25 00:34:09 UTC

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>

Now, I ask you: What purpose is there for this magic NULL behavior? Why would you set a delegate (which the framework asks for a registration dictionary immediately), then explicitly register with no dictionary? What would you expect that to do?

This flagrantly violates the principle of UI design of “if you had to document it, it wasn’t designed intuitively”. The same principle ought to apply to API design. There’s already a Growl_Reregister function which you can use if your delegate’s registration dictionary changes, or if you didn’t have one and you just created it. There’s no reason why you should be able to call Growl_RegisterWithDictionary with no dictionary (for that’s exactly what passing NULL is) and have it do the same thing. What it should do is fail an assertion or something.

(For the record, I’m not calling out anybody else on the Growl Project on this. I designed that API, and I wrote the entirety of what I quoted above. All blame for this rests squarely with me.)

</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.

Conclusion

NULL should not be magic. It should not be special. For the sake of clarity and robustness, It should be illegal.


* 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.

6 Responses to “NULL as a function argument should be illegal”

  1. Evan Says:

    I’ve definitely made this mistake with NSNotificationCenter before — observing for an object I “know” is non-NULL, then being confused when I receive notifications for everything because a buglet elsewhere had nulled the variable. Making it illegal is a great design idea.

  2. David Smith Says:

    AIContactController is (or was, before I changed some of it) a particularly flagrant abuser of this principle. It definitely tends to conceal bugs, and imo NULL arguments should probably assert.

  3. Phil Says:

    I think you miss an important use of NULL, and one i believe to be valid. (example from NSXMLNode)

    -(NSArray *)nodesForXPath:(NSString *)xpath error:(NSError **)error

    When your methods are providing a way to return additional data into a user-specified memory location, being able to optionally say “no thanks, I don’t want that data” is useful, especially when following a standard model (like the NSError one). When it comes to using NULL/nil as a data providing argument, I think you’re right, however.

  4. Peter Hosey Says:

    Phil: Yup, I totally agree—for an output (return-something-by-reference) argument, using NULL to mean “no thanks” should always be allowed.

    I’ll update the post later today, but I’ll point out now that this really only applies to C and its descendants. Other programming languages—those with a first-class sequence type—should simply return everything in a tuple/list/array and let the caller unpack the values and throw away what he doesn’t want. (Fictional Python example: x, y = myView.frameOrigin())

  5. David Smith Says:

    So on a related note, what do you feel about assert()ing pre- and post-conditions for methods? Obviously the technique is a lot more powerful in languages like Eiffel where it can be used at compile time to disallow broken programs, but it still seems potentially useful in other languages.

  6. Colin Barrett Says:

    assert()ing on illegal pre-conditions is extremely useful (although you need to get used to looking at the top of the method for them ;). I’m not sure how asserting on post-conditions would be useful within a method: shouldn’t you assert when you find an inconsistency at any point, not just post-flight?

Leave a Reply

Do not delete the second sentence.