views:

866

answers:

3

Let's say I have an extension method

public static T TakeRandom<T>(this IEnumerable<T> e)
{
    ...

To validate the argument e, should I:

A) if (e == null) throw new NullReferenceException()
B) if (e == null) throw new ArgumentNullException("e")
C) Not check e

What's the consensus?

My first thought is to always validate arguments, so thrown an ArgumentNullException. Then again, since TakeRandom() becomes a method of e, perhaps it should be a NullReferenceException. But if it's a NullReferenceException, if I try to use a member of e inside TakeRandom(), NullReferenceException will be thrown anyway.

Maybe I should peak using Reflector and find out what the framework does.

+3  A: 

For consistency with the Enumerable LINQ operators, throw an ArgumentNullException (not a NullReferenceException).

I would do the validation in the TakeRandom method because the stack trace will then make it clear that it is TakeRandom which objects to being given a null argument.

itowlson
+3  A: 

You should throw an ArgumentNullException. You're attempting to do argument validation and hence should throw an exception tuned to argument validation. NullReferencException is not an argument validation exception. It's a runtime error.

Don't forget, extension methods are just static methods under the hood and can be called as such. While it may seem on the surface to make sense to throw a NullReferenceException on an extension method, it does not make sense to do so for a static method. It's not possible to determine the calling convention in the method and thus ArgumentException is the better choice.

Also, you should not ever explicitly throw a NullReferenceException. This should only be thrown by the CLR. There are subtle differences that occur when explicitly throwing exceptions that are normally only thrown by the CLR.

This is also close to a dupe of the following

JaredPar
+1  A: 

Maybe I am crazy but since it is an argument, I would throw a ArgumentNullException :/

General rule of thumb though is to throw Exceptions that derive from System.ApplicationException when possible. NullReferenceException is something the framework/CLR would throw.

http://msdn.microsoft.com/en-us/library/system.exception(VS.71).aspx

Two categories of exceptions exist under the base class Exception:

The pre-defined common language runtime exception classes derived from SystemException. The user-defined application exception classes derived from ApplicationException.

Chad Grant
" throw Exceptions that derive from System.ApplicationException when possible" - that was Microsoft's original advice for .NET 1.0, but it has now changed and ApplicationException is deprecated. Google for the reasons.
Joe
@Joe please elaborate. On the extremely rare case that I would create my own exceptions .... Why would I not inherit from ApplicationException ? Thanks
Chad Grant
@Deviant: I don't have the article handy, but I believe this is discussed in the Framework Design Guidelines. The original thought behind ApplicationException was that you'd have "catch(ApplicationException ex)" blocks, and do something different on an Application exception vs. a System exception. That turned out not to be useful, and would require Application code to wrap all System exceptions in an Application exception. Framework Design Guidelines also has good rules for when to create your own exceptions, and when to catch exceptions.
John Saunders
@Johm Thanks for the response. I found a link about it http://blogs.msdn.com/kcwalina/archive/2006/06/23/644822.aspxThat's kinda lame that MS trashed it like that. Seemed like a decent idea. Oh well. :(
Chad Grant
@Deviant: The fact was that it turned out _not_ to be a decent idea. It would have required all code to catch exceptions and wrap them in an exception deriving from ApplicationException. If I have a method that validates a parameter is not null, I'd have had the choice of ArgumentNullException, or wrapping ArgumentNullException in ApplicationException. And theree was no other value in separating the types. What would you do differently based on whether an exception was an "Applicaton" vs. a "System" exception?
John Saunders