C#: Should I bother checking for null in this situation?

Question

Lets say I have this extention method:

public static bool HasFive<T>(this IEnumerable<T> subjects)
{
    if(subjects == null)
        throw new ArgumentNullException("subjects");

    return subjects.Count() == 5;
}

Do you think this null check and exception throwing is really necessary? I mean, when I use the Count method, an ArgumentNullException will be thrown anyways, right?

I can maybe think of one reason why I should, but would just like to hear others view on this. And yes, my reason for asking is partly laziness (want to write as little as possible), but also because I kind of think a bunch of null checking and exception throwing kind of clutters up the methods which often end up being twice as long as they really needed to be. Someone should know better than to send null into a method :p

Anyways, what do you guys think?

---

Note: Count() is an extension method and will throw an ArgumentNullException, not a NullReferenceException. See Enumerable.Count<TSource> Method (IEnumerable<TSource>). Try it yourself if you don't believe me =)

---

Note2: After the answers given here I have been persuaded to start checking more for null values. I am still lazy though, so I have started to use the Enforce class in Lokad Shared Libraries. Can recommend taking a look at it. Instead of my example I can do this instead:

public static bool HasFive<T>(this IEnumerable<T> subjects)
{
    Enforce.Argument(() => subjects);
    return subjects.Count() == 5;
}

Answer 1

Yes, it will throw an ArgumentNullException. I can think of two reasons for putting the extra checking in:

• If you later go back and change the method to do something before calling subjects.Count() and forget to put the check in at that point, you could end up with a side effect before the exception is thrown, which isn't nice.
• Currently, the stack trace will show subjects.Count() at the top, and probably with a message with the source parameter name. This could be confusing to the caller of HasFive who can see a subjects parameter name.

EDIT: Just to save me having to write it yet again elsewhere:

The call to subjects.Count() will throw an ArgumentNullException, not a NullReferenceException. Count() is another extension method here, and assuming the implementation in System.Linq.Enumerable is being used, that's documented (correctly) to throw an ArgumentNullException. Try it if you don't believe me.

EDIT: Making this easier...

If you do a lot of checks like this you may want to make it simpler to do so. I like the following extension method:

internal static void ThrowIfNull<T>(this T argument, string name)
    where T : class
{
    if (argument == null)
    {
        throw new ArgumentNullException(name);
    }
}

The example method in the question can then become:

public static bool HasFive<T>(this IEnumerable<T> subjects)
{
    subjects.ThrowIfNull("subjects");    
    return subjects.Count() == 5;
}

Another alternative would be to write a version which checked the value and returned it like this:

internal static T NullGuard<T>(this T argument, string name)
    where T : class
{
    if (argument == null)
    {
        throw new ArgumentNullException(name);
    }
    return argument;
}

You can then call it fluently:

public static bool HasFive<T>(this IEnumerable<T> subjects)
{
    return subjects.NullGuard("subjects").Count() == 5;
}

This is also helpful for copying parameters in constructors etc:

public Person(string name, int age)
{
    this.name = name.NullGuard("name");
    this.age = age;
}

(You might want an overload without the argument name for places where it's not important.)

Answer 2

I think @Jon Skeet is absolutely spot on, however I'd like to add the following thoughts:-

• Providing a meaningful error message is useful for debugging, logging and exception reporting. An exception thrown by the BCL is less likely to describe the specific circumstances of the exception WRT your codebase. Perhaps this is less of an issue with null checks which (most of the time) necessarily can't give you much domain-specific information - 'I was passed a null unexpectedly, no idea why' is pretty much the best you can do most of the time, however sometimes you can provide more information and obviously this is more likely to be relevant when dealing with other exception types.
• The null check clearly demonstrates to other developers and you, a form of documentation, if/when you come back to the code a year later, that it's possible someone might pass a null, and it would be problematic if they did so.
• Expanding on Jon's excellent point - you might do something before the null gets picked up - I think it is vitally important to engage in defensive programming. Checking for an exception before running other code is a form of defensive programming as you are taking into account things might not work the way you expected (or changes might be made in the future that you didn't expect) and ensuring that no matter what happens (assuming your null check isn't removed) such problems cannot arise.
• It's a form of runtime assert that your parameter is not null. You can proceed on the assumption that it isn't.
• The above assumption can result in slimmer code, you write the rest of your code knowing the parameter is not null, cutting down on extraneous subsequent null checks.