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

In my opinion one should check for known conditions that will raise errors later on (at least for public methods). That way it's easier to detect the root of the problem.

I would raise a more informational exception like:

if (subjects == null)
{
     throw new ArgumentNullException("subjects ", "subjects is null.");
}

Answer 2

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 3

It depends on the concrete method. In this case - I think, the exception is not necesary and the better usage will be, if teh extension method can deal with null.

public static bool HasFive<T>(this IEnumerable<T> subjects) {
    if ( object.ReferenceEquals( subjects, null ) ) { return false; }

    return subjects.Count() == 5;
}

If you call "items.HasFive()" and the "items" is null, then is true that items has not five items.

But if you have extension method:

public static T GetFift<T>(this IEnumerable<T> subjects) {
    ...
}

The exception for "subjects == null" should be called, because there is no valid way, how to deal with it.

Answer 4

It always depends on the context (in my opinion).

For instance, when writing a library (for others to use), it certainly makes sense to fully check each and every parameter and throw the appropriate exceptions.

When writing methods that are used inside a project, I usually skip those checks, attempting to reduce the size of the codebase. But even in this case, there might be a level (between application layers) where you still place such checks. It depends on the context, on the size of the project, on the size of the team working on it...

It certainly doesn't make sense doing it for small projects built by one person :)

Answer 5

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.

Answer 6

Yes, for two reasons:

Firstly, the other extension methods on IEnumerable do and consumers of your code can expect yours to do so as well, but secondly and more importantly, if you have a long chain of operators in your query then knowing which one threw the exception is useful information.

Answer 7

If you look at the source to the Enumerable class (System.Core.dll) where a lot of the default extension methods are defined for IEnumerables classes, you can see that they all check for null references with arguments.

public static IEnumerable<TSource> Skip<TSource>(this IEnumerable<TSource> source, int count)
{
    if (source == null)
    {
        throw Error.ArgumentNull("source");
    }
    return SkipIterator<TSource>(source, count);
}

It's a bit of an obvious point, but I tend to follow what I find in the base framework library source as you know that is more than likely to be best practices.

Answer 8

I think it is a good practice to do precondition checks at the top of the function. Maybe it's just my code that is full of bugs, but this practice catched a lot of errors for me.

Also, it's much easier to figure out the source of the problem if you got an ArgumentNullException with the name of the parameter, thrown from the most relevant stack frame. Also, the code in the body of your function can change over time so I wouldn't depend on it catching precondition problems in the future.

Answer 9

The exception that you will get thrown will be an Object reference not set to an instance of an object.

Not the most useful of exceptions when tracking down the problem.

The way you have it there will give you much more useful information by specifically stating that it's your subjects reference that is null.

Answer 10

In my opinion you should check for the null value. Two things that comes to mind.

It makes explicit the possible errors that can happen during runtime.

It also gives you a chance to throw a better exception instead of a generic ArgumentNullException. Thus, making the reason for the exception more explicit.