views:

773

answers:

10

Suppose I have a method that takes an object of some kind as an argument. Now say that if this method is passed a null argument, it's a fatal error and an exception should be thrown. Is it worth it for me to code something like this (keeping in mind this is a trivial example):

void someMethod(SomeClass x)
{
    if (x == null){
        throw new NullArgumentException("someMethod received a null argument!");
    }

    x.doSomething();
}

Or is it safe for me to just rely on it throwing NullException when it calls x.doSomething()?

Secondly, suppose that someMethod is a constructor and x won't be used until another method is called. Should I throw the exception immediately or wait until x is needed and throw the exception then?

+13  A: 

I prefer the ArgumentNull exception over the NullReference exception that not checking the argument would provide. In general, my preference is to always check for nullity before trying to invoke a method on a potentially null object.

If the method is a constructor, then it would depend on a couple of different factors: is there also a public settor for the property and how likely is it that the object will actually be used. If there is a public settor, then not providing a valid instance via the constructor would be reasonable and should not result in an exception.

If there is no public settor and it is possible to to use the containing object without reference to the injected object, you may want to defer the checking/exception until it's use is attempted. I would think that the general case, though, would be that injected object is essential to the functioning of the instance and thus an ArgumentNull exception is perfectly reasonable since the instance can't function without it.

tvanfosson
+1, NullReferenceException is bad.
sixlettervariables
Yeah, it's not a matter of preference. ArgumentNullException is correct and NullReferenceException isn't. NullReferenceException is for when a null object is <i>accessed</i>, such as a field or property of it.
JamesBrownIsDead
+9  A: 

I always follow the practice of fail fast. If your method is dependent on X and you understand X might be passed in null, null check it and raise the exception immediately instead of prolonging the point of failure.

Chris Marisic
+6  A: 

I prefer the explicit exception, for these reasons:

  • If the method has more than one SomeClass argument it gives you the opportunity to say which one it is (everything else is available in the call stack).
  • What if you do something that may have a side effect before referencing x?
Joel Coehoorn
+2  A: 

If you program defensively you should fail fast. So check your inputs and error out at the beginning of your code. You should be nice to your caller and give them the most descriptive error message you can.

Aaron Fischer
+3  A: 

It is better to throw the ArgumentNullException sooner rather than later. If you throw it, you can provide more helpful information on the problem than a NullReferenceException.

g .
+2  A: 

1- Do it explicitly if it you do not want Null value. Otherwise, when someone else will look your code, they will think that passing a Null value is accepted.

2- Do it the more rapidly you can. This way you do not propagate the "wrong" behavior of having a Null when it's not supposed.

Daok
+4  A: 

You should explicitly throw an ArgumentNullException if you are expecting the input to not be null. You might want to write a class called Guard that provides helper methods for this. So your code will be:

void someMethod(SomeClass x, SomeClass y)
{
    Guard.NotNull(x,"x","someMethod received a null x argument!");
    Guard.NotNull(y,"y","someMethod received a null y argument!");


    x.doSomething();
    y.doSomething();
}

The NonNull method would do the nullity check and throw a NullArgumentException with the error message specified in the call.

siz
How about the stacktrace in the exception?It would contain the Guard method NotNull, which is only noise, and might cause confusion. Is there some way of avoiding this?
khebbie
+4  A: 

I'd prefer the parameter check with the explicit ArgumentNullException, too.

Looking at the metadata:

 //
    // Summary:
    //     Initializes a new instance of the System.ArgumentNullException class with
    //     the name of the parameter that causes this exception.
    //
    // Parameters:
    //   paramName:
    //     The name of the parameter that caused the exception.
    public ArgumentNullException(string paramName);

You can see, that the string should be the name of the parameter, that is null, and so give the developer a hint on what is going wrong.

BeowulfOF
Thanks! I suppose I should have spent more time reading the documentation on the constructor. :-/
Jason Baker
+3  A: 

I agree with the idea of failing fast - however it is wise to know why failing fast is practical. Consider this example:

void someMethod(SomeClass x)
{    
    x.Property.doSomething();
}

If you rely on the NullReferenceException to tell you that something was wrong, how will you know what was null? The stack trace will only give you a line number, not which reference was null. In this example x or x.Property could both have been null and without failing fast with aggressive checking beforehand, you will not know which it is.

Andrew Hare
A: 

I'll probably be downvoted for this, but I think completely different.

What about following a good practice called "never pass null" and remove the ugly exception checking?

If the parameter is an object, DO NOT PASS NULL. Also, DO NOT RETURN NULL. You can even use the Null object pattern to help with that.

If it's optional, use default values (if your language supports them) or create an overload.

Much cleaner than ugly exceptions.

andrecarlucci