Extension methods: If you have a shiny new hammer

March 20, 2009 at 10:40 AMAndre Loker

If you have a shiny new hammer, every problem looks like a nail they say. Although C# 3.0 extension methods are not that new anymore, this saying still applies. I ran across the announcement of the Generic Extension Methods Project. While I think a repository of useful extension methods sounds like a cool idea, one piece of code in that said announcement made me smile slightly:

   1: // IsNotNull is an extension method
   2: if(row.IsNotNull())
   3: {
   4:     row["First Column"] = "some value";
   5: }

Call me old fashioned, but I personally prefer the good old null-check a lot:

   1: // Pure and simple
   2: if(row != null)
   3: {
   4:     row["First Column"] = "some value";
   5: }

something != null  is an expression every developer understands. If I don’t know about extension methods or don’t know in particular that IsNotNull() is an extension method, I’d ask myself: “What does a method IsNotNull() do? Is nullness meant to be something different here? It has to, because calling a method on a null-variable would cause a NullReferenceException, wouldn’t it?”.

One inherent semantic of C# is that if you call a method on a null reference, it will crash. IsNotNull() is therefore completely counterintuitive. Either you force the reader to consider the possibility that the method might be an extension method and therefore you could call that method on null. Or you help those readers by preceding all usages of IsNotNull() with comments like “Note, this is an extension method”. Yuck!

But maybe this IsNotNull() implementation does a bit more for DataTableRows than just checking them to be not null. If it does, it’s name is misleading.

So you see: using IsNotNull() adds nothing but confusion to the code.

My personal recommendations regarding extension methods:

  • Don’t use them just because you can.
    • The ultimate goal should be to write simple, readable, comprehensible code. If an extension method helps – use it. If it doesn’t or is even counterproductive – don’t!
  • If there is a simpler concept built into the language, prefer that over extension methods. Some (partly far-fetched) examples:
    • prefer something != null over something.IsNotNull()
    • prefer if(something)… over if(something.IsTrue())…
    • prefer x = a + 1 over x = a.PlusOne()

A brief review

That being said, here’s a brief review of the extension methods that are found in the project as of now:

CollectionExtensions

FirstItem returns the first item of a list or array or null if there is no such item. Isn’t that what FirstOrDefault already does?

ReflectionExtensions

CreateInstance – which extends Type - comes in two flavours:

The first one tries to create an instance using the default constructor:

   1: public static T CreateInstance<T>(this System.Type type) where T : new()

Although the documentation says that you could use it like “typeof(MyObject).CreateInstance()” this is not true – you need to provide something for T. Not only do you need to provide the type twice (as the extended object and as T), you also end up with possible constructs like:

   1: typeof(string).CreateInstance<Version>();

This happily creates an instance of Version. What’s the use of extending Type then? Just use Activator – it’s simple and works:

   1: Activator.CreateInstance<Version>()

The other overload of CreateInstance additionally allows for constructor arguments being passed. What’s been said above still counts, typeof(string).CreateInstance<Version>(1,2,3,4) is awkward.

Just as a side note: what’s the use of a generic object creation method anyway? The documentation of Activator.CreateInstance<T> puts it nicely:

In general, there is no use for the CreateInstance in application code, because the type must be known at compile time. If the type is known at compile time, normal instantiation syntax can be used.

That is: if somewhere in the code I am able to say Activator.CreateInstance<Foo>() – why don’t I just say new Foo() in the first place? What I’d find more useful is something like this:

   1: public static T InstantiateAs<T>(this Type type) {
   2:   return (T) type.Instantiate();
   3: }
   4:  
   5: public static object Instantiate(this Type type) {
   6:   return Activator.CreateInstance(type);
   7: }

So you could say:

   1: Type type = ReadSomeTypeFromConfig(); // returns e.g. typeof(ServiceImpl)
   2: IService myService = type.InstantiateAs<IService>();

And likewise add overloads that accept constructor arguments.

ValidationExtensions

This class contains extension methods such as IsNull or IsNotNull – which I don’t like at all as explained above.

Additionally, you’ll find assertion methods like

   1: public static void AssertParameterNotNull(this object value, 
   2:                                           string message, 
   3:                                           string name)

It’s probably a matter of taste whether you like it or not. Again, I prefer a more explicit way. I prefer not to allow method calls on possible null-values and go with:

   1: Assert.ArgumentIsNotNull(theArgument, "theArgument", "Boy, that arg can't be null!")

Then we have AssertEquals which is supposed to be used like this:

   1: someValue.AssertEquals<MyException>(someOtherValue, "Some message");

That is, MyException is thrown if someValue does not equal someOtherValue. I can’t help it, it feels weird.

Next one:

   1: public static bool IsEmpty(this string value)
   2: public static bool IsNotEmpty(this string value)

Looks reasonable, doesn’t it? The problem with these methods is that their names are very misleading:

   1: /// <summary>
   2: /// Tests if the string is empty.
   3: /// </summary>
   4: /// <param name="value">The string to test.</param>
   5: /// <returns>True if the string is empty.</returns>
   6: public static bool IsEmpty(this string value)
   7: {
   8:     return value.Trim().Length == 0;
   9: }
  10:  
  11: /// <summary>
  12: /// Tests if the string is not empty.
  13: /// </summary>
  14: /// <param name="value">The string to test.</param>
  15: /// <returns>True if the string is not empty.</returns>
  16: public static bool IsNotEmpty(this string value)
  17: {
  18:     return value.Trim().Length > 0;
  19: }

According to those methods, “    “.IsEmpty() == true. Which is confusing because string.IsNullOrEmpty(“    “) returns false. I’d heavily suggest to rename those methods and be clear about the behaviour in the documentation.

Finally, there are a bunch of methods called IsEmpty and IsNotEmpty that work on different collection types (why aren’t they placed in the CollectionExtensions class?). While I find those methods useful there is an extreme amount of duplication. All those methods can be covered by these to little guys:

   1: public static bool IsEmpty<T>(this T collection) where T : ICollection {
   2:   return collection.Count == 0;
   3: }
   4:  
   5: public static bool IsNotEmpty<T>(this T collection) where T : ICollection {
   6:   return !collection.IsEmpty();
   7: }

Because all of the types handled in the library (ICollection, ICollection<T>, IList, IList<T>, IDictionary, IDictionary<K, T>, Array) inherit ICollection. Why did I make IsEmpty generic instead of just passing an ICollection? It’s just a little trick: if for whatever reason you have a value type that implements ICollection invoking IsEmpty(ICollection) on it would cause it to be boxed to treat it as an ICollection. By making IsEmpty generic with a constrained this boxing will not occur. OK, it’s not likely to happen, but I prefer this style. It explicitly says: “It operates on everything that can behave like an ICollection” rather than “It operates on an ICollection”. Sometimes it’s in the details.

Posted in: C# | Patterns

Tags: ,