Extension method madness

Published Wed, Jun 23 2010 11:38

Even though I haven’t been as active as I wished, I have still managed to get some time to read several interesting blog posts lately. Unfortunately, it seems like two guys that I have a lot of respect for (though I haven’t had the pleasure of meeting them in person) have gone into the “dark side”. I’m referring to Phil Haack and Oren Eini (aka Ayende). The problem: using an extension method for checking for an empty enumeration.

It all started with Phil’s post, where he presented an extension method for testing for an empty IEnumerable. Here’s the code he ended up with:

public static bool IsNullOrEmpty<T>(this IEnumerable<T> items) {
    return items == null || !items.Any();
}

Phil really explains why using the Any is a good option (especially when compared to Count) and I do agree with that part. Now, the problem here is that Ayende picked it up and improved it so that you don’t end up “loosing” items when you can’t go several times over an IEnumerable. Unfortunately, both of them missed the null check. Don’t understand what I mean? In that case, do take some time and look really carefully at Phil’s initial code. Do you see anything wrong? No? Let me ask you a question: what’s the expected result of the following code:

IEnumerable<Int32> nullCll = null;
var isEmpty = nullCll.IsNullOrEmpty();

In my opinion, it should throw a null reference exception. But that won’t happen in this case. Don’t you find that weird? I do and I think extension methods should behave the same way instance methods. And that really means null exceptions shouldn’t be “replaced” with Boolean values.

Btw, and that’s just my opinion, the helper method should really be a simple helper method (ie, not an extension method). Just because we have extension methods, it doesn’t meant they’re the solution to all the extension problems we have…

by luisabreu

Filed under: C#

Comments

# Carles said on Wednesday, June 23, 2010 6:01 AM

If you are testing whether nullCll is null it should return true not throw an exception. I would consider that bad behavior. It's the same as with string.IsNullOrEmpty...

# luisabreu said on Wednesday, June 23, 2010 6:09 AM

No, it is not because you *use* String.IsNullOrEmpty (static method called over a type) and not nullStr.IsNullOrEmpty (instance method which throws null ref exception).

# Joaquim Rendeiro said on Wednesday, June 23, 2010 11:51 AM

I'd find it weird if the name of the method was simply IsEmpty().

IsNullOrEmpty() expresses perfectly the intent, you're getting exactly what you signed up for.

# Jason said on Wednesday, June 23, 2010 11:54 AM

I see your point if it actually was an instance method. (On an instance) However, since an extension method is not much more than syntax over a static method

Personally, I like the readability of the the extension method better.

Extensions.IsNullOrEmpty(nullCll);

is not a clean to read as

nullCll.IsNullOrEmpty();

I am against introducing _too many_ of these helpers because it can get pretty messy in the intellasense(sp?) view

(EX: if you use something like unit test spec helper extensions (ShouldEqual<T>, ShouldNot... etc) it can get a little annoying.

# luisabreu said on Wednesday, June 23, 2010 12:42 PM

It's a question of semantics. I've always learned that extension methods should behave like instance methods. and that really means no null checking. I do believe that this sort of approach is a bad practice which should be banned for good. btw, you could always check for null and throw a null reference exception (I'm under the impression that enumerable methods do this). However, I believe that the best approach is to really do no checks because the previous check does "simulate" normal instance behavior but it changes it slightly (just look at the stack call to see what i mean)...

# LA.NET [EN] said on Tuesday, August 31, 2010 4:02 AM