Saturday, April 4, 2009

Exceptional APIs

Axioms for public API design:

Axiom 1
NullReferenceExceptions that come out of your code ARE YOUR FAULT!

Axiom 2
The fewer exceptions your code can possibly throw, the better!

Applications of these axioms:

Let's say we're writing a public API, you and I. In it, let's say we have the following awesome method:
public void Foo(string s) {
    Bar(s.Length);
}
You will notice that we dereference s to get its Length property. If some misguided user of ours were to pass null to Foo, an NRE would pop out of our code like an overweight stripper out of a wedding cake. Awkward! And then the stack trace would get passed all over school and all the kids would laugh at us.

Lemma 1
From axiom 1 it follows: Null-check EVERYTHING YOU DEREFERENCE unless you know where it came from.

We could do a conditional dereference:
public void Foo(string s) {
    if (s != null) Bar(s.Length);
}
Or we could verify the argument:
public void Foo(string s) {
    if (s == null) throw new ArgumentNullException("s");
    Bar(s.Length);
}
If our method absolutely needs to dereference the object in order to do its job, then argument verification is the way to go. This may seem like trading one exception type for another, but it's really not. Argument exception types are perfectly acceptable - they inform our misguided user what went wrong and how to make it right. On the other hand, NREs mean that we didn't verify the argument or null-check before dereferencing. And they mean that all the kids will laugh at us.

Let's say that we decide on argument verification. Now let's say that we want to add a convenience overload:
public void Foo() {
    Foo("");
}
Looks good, right? WRONG!

Lemma 2
From axiom 2 it follows: Use 'null' as a sentinel between overloads for the default value when null is not an otherwise permissible value.

My favorite non-fiction book of all time, Framework Design Guidelines, specifically says not to do this. It is wrong. Well, sort of. FDG advises against using null as a "magic" sentinel, period. I argue that sentinel null is correct for parameters which are omitted in convenience overloads. Here is why:

To recap, our API currently looks like this:
public void Foo() {
   Foo("");
}

public void Foo(string s) {
    if (s == null) throw new ArgumentNullException("s");
    Bar(s.Length);
}
Let's consider the possible ways our misguided user can use this API. If he or she calls the convenience overload, or passes a string literal, everything's honkey dorey:
Foo(); // This is fine
Foo("How the hell do I use this API?"); // This too is just fine

But if misguided user passes null to Foo(string), or passes a variable which may be null, things aren't so rosey:
Foo(null); // Exception city!
Foo(someString); // It depends...

How is our misguided user to know what is and is not a safe call? They could look at our documentation. Assuming we wrote any. And assuming we correctly documented all of the parameters which need to be non-null. Or they could test passing null and see if it throws.

(As a side note, I wonder all the time about whether I can pass null to an API. Documentation is usually no help and if I see a parameterless overload, I generally assume that I can)

More likely, they will do none of the above and just write something resembling the last example call, passing a variable. A variable which is usually not null. A variable which is never null during testing. But a variable which, when released into the wild may, under some unforeseen circumstance, be null. Then all the kids would cry.

The solution:
public void Foo() {
    Foo(null);
}

public void Foo(string s) {
    if (s == null) s = "";
    Bar(s.Length);
}
All possible inputs to this API are valid and there is zero change of an exception. This is better!

Some may complain about semantics purity or somesuch. They are wrong.

So, if you have some public API which takes a parameter that a) is not allowed to be null, and b) has a default value for the purpose of convenience overloads, use the sentinel null. Just do it.

7 comments:

Anonymous said...

Actually the FDG explicitly recommend allowing null to be passed for optional arguments (see p.126, 2nd ed.)

Anonymous said...

I think you glossed over an important point: Is Foo() appropriate for your API?

Let's go back to original method:

public void Foo(string s) {
Bar(s.Length);
}

Before you add an overload that takes no parameters you need to decide if that overload makes sense. Does Foo() actually have some valid meaning to you? Don't just add it as a convenience for your callers. The presence or absence of this method actually helps them learn how this API works.

If it does make sense to include this overload then I could see proceeding as you do, however in those cases you normally know the default action and can avoid changing the incoming parameter.

public void Foo(string s) {
if (s == null) Bar(); // or Bar(0) or BarEmpty()
else Bar(s.Length);
}

If Foo() is obviously meaningless, then don't include it in your API and leave the ArgumentNullException in your implementation.

public void Foo(string s) {
if (s == null) throw new ArgumentNullException("s");
Bar(s.Length);
}

Anonymous said...

Everything could be avoided changing something like this:

public void Foo(string s) {
Bar(s.Length);
}

to something like this:

public void Foo(int length) {
Bar(length);
}

Scott said...

I think I should clarify that the Foo methods are purely an example. No API in its right mind would actually include such members, for the very reasons the last two commenter articulate. They are purely devices to demonstrate my point. If you'd like a real life example, look at the constructors in this class: http://anonsvn.mono-project.com/viewvc/trunk/mono-upnp/src/Mono.Upnp/Mono.Upnp.Client/Mono.Upnp/UpnpClient.cs?view=markup

Sandro Magi said...

Your advice to avoid exceptions is sound. Basically, you're recommending that functions be "total", rather than "partial". Using exceptions can lead to unpredictable behaviour when exceptions are not checked, as in C#, since we don't know whether any given composition of functions may catch all exceptions.

I'm not sure I agree with axiom 1 however. Personally, I try to make my programs total, and ANY exceptions are thus avoided where possible. I generally don't care much which exception was thrown, as I use exceptions only for aborting a user-triggered computation and communicating any input errors back to the user.

Thus, my requirement is only that exceptions have end-user readable error messages. Of course, this applies to code that I've written, but I try to adapt third-party code to this convention as well.

I think ultimately totality is the more important program property.

Kannan Goundan said...

Generally, yes, it's nice to have total functions. But given an existing function definition (i.e. the one you started with) there are two ways of achieving this.

One is to modify the function to cover the entire domain (as you recommended). This probably means adding special cases, adds logical complexity to your program.

The other option is to refine the domain. Ideally, this would mean fixing the static type of the parameters (e.g. non-nullable types). But since you can't really do this, the next best thing is to signal an error at run-time (by throwing an exception).

With your "Foo" function, a null-pointer bug in client code may manifest itself as an array-out-of-bounds error somewhere down the line. This makes the bug harder to track down. Throwing an exception right away would detect the error closer to its root cause.

Sandro Magi said...

The other option is to refine the domain. Ideally, this would mean fixing the static type of the parameters (e.g. non-nullable types). But since you can't really do this [...]

You can actually get pretty close with C#. See the NonNull implementation in my Sasa library. As long as you stick to the convention of only decorating method arguments with NonNull, it's a fairly safe, low-overhead technique of assuring absence of nulls.

It's too bad MS really missed the boat here. They had a good idea adding System.Nullable, but it should have been a proper option/maybe type, and they should have ensured reference types had non-null values by default, and not vice-versa.