Tuesday 12 August 2008

A designer's thought process

It was the ForEach method that got me started. List<T> has a ForEach method that calls a delegate on each of its items, and many people are asking why there's no ForEach method on the Enummerable class in the BCL (the answer has to do with encouraging the side-effect-free programming style beloved by Functional-Programming purists). It's not a big deal really. You can knock one up in seconds:

public static class EnumerableExtensions
{
    public static void ForEach<T>(this IEnumerable<T> sequence, Action<T> action)
    {
        foreach (var item in sequence)
        {
            action(item);
        }
    }
}

I was using that very extension method just a few minutes ago, writing code like this.

ViewModel.DataSetList.ForEach(ds => ds.ClearDetails());

As I did a micro code-review before checking in my work, I asked myself: What happens if DataSetList is null? Oops. The code will blow up with a NullReferenceException. Hmm. But at which point? Interestingly, it's not in the line where I'm using the ForEach method, but inside the ForEach method that the exception will be thrown. It would be different if ForEach was defined as an instance method on DataSetList's type; but because ForEach is an extension method, it can safely be called on a null reference: null gets passed to the parameter marked with the this keyword (sequence in this case - then the NullReferenceException will happen in the foreach block). This property of extension methods has inspired a number of interesting techniques, the Null-propagating extension method being one of them.

But how should I fix my code?

Cogs turning, gears grinding.

I could modify ForEach to do nothing if it is passed a null reference in the sequence parameter. That will do the trick for this case, but what effect will it have on the rest of the code-base? Suppose that somewhere else I have an enumerable variable that should always have a value, but a bug means that occasionally it is null. With ForEach written as it is, I'm going to get a glaring unhandled exception dialog alerting me to the bug; if I make the change I've just suggested, all such bugs are going to be silently masked. Not a good idea, methinks.

OK then. How about creating an overload of the ForEach method that takes a parameter to control whether it ignores nulls or throws an exception? Ugh! Or as Brad Abrams puts it (Framework Design Guidelines pp187) "An API [like that] usually reflects the inability of the framework designer to make a decision."

Well, I finally came to my decision. Not so long ago, I was reading in the excellent book The Pragmatic Programmer about the principle of orthogonality. Applying this in miniature to my case, I realised that I needed to separate the responsibility of deciding what to do with null sequences from the responsibility of acting on the sequence. The answer was to create another extension method:

public static IEnumerable<T> EmptyIfNull<T>(this IEnumerable<T> sequence)
{
    return sequence ?? Enumerable.Empty<T>();
}

With that in place, I can fix my code like this:

ViewModel.DataSetList.EmptyIfNull().ForEach(ds => ds.ClearDetails());

Neat, tidy, intention-revealing, and applicable in so many other cases.

[I should note that the most obvious way of solving the problem is to wrap the troublesome line of code in an if (DataSetList != null) block. That didn't occur to me until I was writing up this post. I'm not sure what that says about me.]

[Update: Made EmptyIfNull() more compact thanks to a reminder about null-coalescing operator from Jacob Carpenter (see the comments)]

2 comments:

Anonymous said...

Of course you mean:

return sequence ?? Enumerable.Empty<T>();

Unknown said...

Jacob,
Of course that's what I meant to write. The extra curly braces where just a slip of the fingers. ;-)

Sam

Post a Comment