7

Possible Duplicate:
C#: Best practice for validating “this” argument in extension methods

I'm ambivalent about a design choice, and would like to hear the opinions of the SO community. The example I bring up here is just one possible case where this design choice has to be made - in reality, there might be many more cases. Answers are welcome both to this specific case and to a more general approach, and guidelines on how to make the decision in a specific case are also appreciated.

Basically, I want to know how to think about this: When writing an extension method that doesn't intrinsically fail if a null reference is passed as the this instance, should a null check be performed on the argument or not?

Example:

I'm writing an extension method on IEnumerable<T> that will iterate through the collection and performe some Action<T> - basically, this is what it'll do:

public static void Each<T>(this IEnumerable<T> collection, Action<T> action)
{
    foreach (var t in collection)
    {
        action.Invoke(t);
    }
}

What I cannot decide about, is what this extension method should do if null is passed into either parameter. If I do not add any null checks, I will get a NullReferenceException on action.Invoke(T), but if collection is null the for loop will just silently do nothing (and no exception will be thrown even if action is also null...).

I am quite decided to add a null check for action, so I can throw an ArgumentNullException instead of the NullReferenceException. But what do I want to do about the collection?

Option 1: Add a null check, and throw ArgumentNullException.

Option 2: Just silently let the method do nothing.

Which will be more useful where I might want to use the method in the future? Why?

Community
  • 1
  • 1
Tomas Aschan
  • 58,548
  • 56
  • 243
  • 402
  • 2
    If collection is null you will also get a NullReferenceException from the foreach loop. – m0sa May 31 '11 at 21:04
  • 1
    Just a by the way: You do know that LINQ provides a ForAll (or something like that) already, that does just what you want it to (as far as I can see?). Not that it has anything do to with your question, and this might just be an example from your part, but you might try to see what the LINQ-extension-methods does when you give them null, and maybe copy that? Follow MS' patterns when writing C#? – Alxandr May 31 '11 at 21:06
  • @bzlm: How does that work? Or is it only for collections/enumerables? – Alxandr May 31 '11 at 21:06
  • 1
    @Alxandr: `ForAll` is for parallel enumerable only. Microsoft intentionally left out `ForEach` from LINQ for `IEnumerable`. – Rick Sladkey May 31 '11 at 21:08
  • @Alxandr It's usually implemented as a thin wrapper around `string.IsNullOrEmpty()`, and I've yet to see a code base which doesn't have it. :) But it answers @Tomas' question in the general case (ie. sometimes the this argument *can* legally be null). So it limits this question to the exact case presented. @Rick, it's for `IList`. – bzlm May 31 '11 at 21:09
  • @m0sa: You are right - the implementation I had was only run from a test, involving a call to `((IEnumerable)null).Each(d => d.DoSomething())` that I expected to throw `ArgumentNullException` (since I wanted it to, and do TDD). When that failed, I incorrectly inferred that no exception was thrown. – Tomas Aschan May 31 '11 at 23:36

4 Answers4

12

Microsoft throws an ArgumentNullException if the collections invoked in LINQ are empty. This is really more a matter of style, but is consistent with the way extension methods are supposed to behave.

@m0sa is right though in that you'll get a null reference from your foreach, but I would say check up top and throw ArgumentNullException. That way you'll be on par with what LINQ does.

For example, if you look at Any() in a decompiler you see:

public static bool Any<TSource>(this IEnumerable<TSource> source)
{
    if (source == null)
    {
        throw Error.ArgumentNull("source");
    }
    using (IEnumerator<TSource> enumerator = source.GetEnumerator())
    {
        if (enumerator.MoveNext())
        {
            return true;
        }
    }
    return false;
}
bzlm
  • 9,626
  • 6
  • 65
  • 92
James Michael Hare
  • 37,767
  • 9
  • 73
  • 83
  • In this example, this makes perfect sense. But what about `.IsNullOrEmpty()`? In that case, it really wouldn't make sense to throw on null... Would that then be bad practice for extension methods? – Tomas Aschan May 31 '11 at 23:50
2

You should definitely do null checking and throw ArgumentNullException to avoid hard to understand NullReferenceExceptions inside your code.

In general I would avoid to treat null as en "empty" IEnumerable<T>. Just use Enumerable.Empty<T>() instead to avoid special cases for a null collection.

If you decide to do null checking in your extension method you should consider doing that "eagerly". If you are using yield return inside your extension method none of it will actually be evaluated until iteration begins. You split your extension method into two parts. The "eager" part where arguments are checked and the "lazy" that yield return elements.

Martin Liversage
  • 104,481
  • 22
  • 209
  • 256
0

First of all: I'm quite sure that there will be an NullReferenceException if collection is null. So that part of your question is no problem at all. Regarding the rest: Do you gain anything by checking for null and throwing a different exception? In my opinion: no! So I would not clutter my code with checks that do not help at all.

Achim
  • 15,415
  • 15
  • 80
  • 144
0

If the collection argument was null I would throw a NullReferenceException. This comes from thinking about how it would behave if it was null, and Each<T> happened to be just a regular method--a NullReferenceException being thrown is what I would expect to happen.

EDIT: Based on Martin's comment and some further research on this, I take back what I said. It seems that a NullReferenceException shouldn't be thrown in this case, as Microsoft recommends using an ArgumentNullException instead.

ataddeini
  • 4,931
  • 26
  • 34
  • You are not supposed to throw a `NullReferenceException` from your code. The runtime will throw it if a `null` reference is dereferenced. If you want to avoid that (any many do) you should check method arguments and throw `ArgumentNullException` if necessary. – Martin Liversage May 31 '11 at 21:21
  • @Martin Liversage: Yes, [it appears that's correct](http://blogs.msdn.com/b/jaredpar/archive/2010/06/28/do-not-throw-a-nullreferenceexception-when-validing-this-in-an-extension-method.aspx). My mistake. – ataddeini Jun 01 '11 at 02:21