Extension Methods Should Behave Like Real Methods

The other day I was involved in two discussions about Extension Methods in C#. The second was an extension (pun intended) of the first as a by-product of me tweeting about it. My argument, in both cases, is that the observable outcome of invoking an extension method with a null “this” argument should be the same as if that method was directly implemented on the class.

This != null

What kick-started the whole discussion was a change in the implementation of an extension method I wrote for the System.String class that checks if a string value [1] is empty:-

public static bool IsEmpty(this string value)
{
  return (value.Length == 0);
}

It had been replaced with this implementation:-

public static bool IsEmpty(this string value)
{
  return String.IsNullOrEmpty(value);
}

Ironically, the reason it was changed was because it caused a NullReferenceException to be thrown when provided with a null string reference. This, in my opinion, was the correct behaviour.

Whenever I come across a bug like this I ask myself what is it that is wrong - the caller or the callee? There are essentially two possible bugs here:-

1. The callee is incorrectly implemented and does not support the scenario invoked by the caller
2. The caller has violated the interface contract of the callee by invoking it with unsupported arguments

The code change was based on assumption 1, whereas (as implementer the extension method) I knew the answer to be 2. In this specific case the bug was still mine and down to me misinterpreting how “empty” string values are passed to MVC controllers [2].

My rationale for saying that the “this” argument cannot be null and that the method must throw, irrespective of whether the functionality can be implemented without referencing the “this” argument or not, is that if the method were to be implemented directly in the class in the future, it would fail when attempting to dispatch the method call. This could be classified as a breaking change if you somehow relied on the exception type.

Throw a NullReferenceException

This leads me to my second point and the one that came up via Twitter. If you do decide to check the “this” argument make sure you throw the correct exception type. Whilst it might be common to throw some type of ArgumentException, such as a ArgumentNullException when validating your inputs, in this case you are attempting to emulate a real method and so you should throw a NullReferenceException as that is what would be thrown if the method was real:-

public static bool MyMethod(this object value, . . .)
{
  if (value == null)
    throw new NullReferenceException();
  . . .
}

The reason I didn’t explicitly check for a null reference in my extension method was because I was going to call an instance method anyway and so the observable effect was the same. In most cases this is exactly what happens anyway and so doing nothing is probably the right thing to do - putting an additional check in and throwing an ArgumentNullException would be wrong.

Test The Behaviour

As ever if you’re unsure and want to document your choice then write a unit test for it; this is something I missed out originally. Of course you cannot legislate for someone also deleting the test because they believe it’s bogus, but it should at least provide a speed bump that may cause them to question their motives.

In NUnit you could write a simple test like so:-

[Test]
public void if_empty_throws_when_value_is_null()
{
  string nullValue = null;

  Assert.That(() => nullValue.IsEmpty,
         Throws.InstanceOf<NullReferenceException>());
}

Is It Just Academic?

As I’ve already suggested, the correct behaviour is the likely outcome most of the time due to the very nature of extension methods and how they’re used. So, is there a plausible scenario where someone could rely on the exception type to make a different error recovery decision? I think so, if the module boundary contains a Big Outer Try Block. Depending on what role the throwing code plays a NullReferenceException could be interpreted by the error handler as an indication that something logical has screwed up inside the service and that it should terminate itself. In a stateful service this could be an indication of data corruption and so shutting itself down ASAP might be the best course of action. Conversely an ArgumentException may be treated less suspiciously because it’s the result of pro-active validation.

 

[1] If you’re curious about why I’d even bother to go to such lengths (another pun intended) then the following older blog posts of mine may explain my thinking - “Null String Reference vs Empty String Value” and “Null Checks vs Null Objects”.

[2] It appears that you get an empty string on older MVC versions and a null reference on newer ones. At least I think so (and it’s what I observed) but it’s quite hard to work out as a lot of Stack Overflow posts and blogs don’t always state what version of MVC they are talking about.