# Thursday, 20 September 2007
This came up at work this week, so I thought I'd disgorge my views on the issue.  The question at hand is whether or not every method needs to validate its input parameters. 
For example, if I have a method

    public class Example

    {

        public int DoThingy(SomeObject obj)

        {

            return obj.GetImportantValue();

        }

    }


This method depends on the value of obj not being null.  If a caller passes a null, the framework will, on my behalf, throw a NullReferenceException.  Is that OK?  Sometimes it might be, and we'll come back to that.
The alternative is for me to validate my input parameters thusly:

        public int DoThingy(SomeObject obj)

        {

            if (obj == null)

                throw new ArgumentNullException("obj");

 

            return obj.GetImportantValue();

        }


The discussion we got into was wether this was really necessary or not, given that the Example object only got used in one place, and it would require extra code to validate the parameters.  The assertion was made by more than one person that the NullReferenceException was just fine, and that there was no need to do any validation. 
My $.02 on the matter was (and is) that if you don't do parameter validation, you are not upholding and/or communicating your contract.  Part of the contract implied by DoThingy() is that you have to pass a non-null SomeObject reference.  Therefore, in order to properly communicate your contract, you should throw an ArgumentNullException, which informs the caller exactly how they have violated there half of the contract.  Yes, it's extra code.  No, it may never be necessary.  A whole different subject is whether of not "fewest possible lines of code" is a design goal.  I'm going to avoid going there just now.

That said, there are obviously mitigating circumstances that apply.  If the object in question is really only called in one place, upholding the contract is less of a concern, since you should be able to write tests that cover the right cases to make sure the caller never passes null. Although that brings up a separate issue.  If the method only had one caller, which is it in a separate object at all?  Again, we'll table that one.  In addition, since in this particular case the DoThingy() method only takes one parameter, we don't have to wonder to hard when we get a NullReferenceException where the culprit is. 

The other issue besides contract is debugging.  If you don't check your input, and just let the method fail, then the onus is on the caller to figure out what the problem is.  Should they have to work it out?  If the method took 10 parameters, all reference types, and I let the runtime throw NullReferenceException, how long will it take the consumer to find the problem?  On the other hand, if I validate the parameters and throw ArgumentNullException, the caller is informed which parameter is null, since the onus was on me to find the problem. 

One last reason...  If I validate my input parameters, it should leave me with fewer failure cases to test, since the error path is potentially much shorter than if the runtime handles it.  Maybe not much of a savings, but it's there.

Thoughts?

Thursday, 20 September 2007 16:25:21 (Pacific Daylight Time, UTC-07:00)
In your example, if the argument is that the situation is so simple, then it should be simple to handle (with an ArgumentException) the null case. I'm with you... Instead of "good enough", we should strive for "ideal".
Thursday, 20 September 2007 16:28:47 (Pacific Daylight Time, UTC-07:00)
And the security argument, since it's close enough to apples-apples, heh:

Input prarmeter and value testing at all layers is important, becuase your software may not be used or accessed in the way you inteneded. Never assume that one check is good enough. Check everywhere, and you get the added benefits you mentioned. Of course, this is the data integrity and 0wn3d app argument, but that happens a lot, so thinking like the bad guy is important and dovetails well. Or at least I think it does. :)

Thursday, 20 September 2007 16:30:30 (Pacific Daylight Time, UTC-07:00)
Also - getting prompted to authenticate on the comments page of your site and getting errors from dasBlog when posting the comment, although it seems to have saved. Just FYI. :)
Thursday, 20 September 2007 16:43:50 (Pacific Daylight Time, UTC-07:00)
The security argument is also a good one, I hadn't even considered that.
I'll check on the authentication and errors. My hosting provider (once again) screwed up all the permissions on my site, and I'm still tracking down problems.

Thanks.
Thursday, 20 September 2007 20:54:10 (Pacific Daylight Time, UTC-07:00)
I usually follw the rule, that if it's public method in a public type validate. Null checking is just one case, and in the simplistics example we can as well forego it. But in huge apps it's better to layer out the actual func in a inner/internal class and do validations, security etc in the public function
Rajiv
Thursday, 27 September 2007 05:26:23 (Pacific Daylight Time, UTC-07:00)
I've thought about this recently, too, and I am in the same boat as Rajiv -- any method which can be called by an other class (i.e. public or protected) must do validation. Private methods can usually avoid it -- especially if those private methods are helpers for public methods (which already did the validation).
Tuesday, 16 October 2007 05:44:58 (Pacific Daylight Time, UTC-07:00)
I agree that throwing this exception saves a lot of debugging time as well.
If the situation is

void DoSomething(someObj obj)
{
return DoSomethingElse(obj) // which in turn may call another one
}

in that case, if we have a NULL value, we may end up with an exception in a place that could be misguiding.
Tuesday, 16 October 2007 08:08:51 (Pacific Daylight Time, UTC-07:00)
The object is question is only called in one place...now. Why not enforce Design By Contract now, and save headaches down the road.

Sean Chambers had a pretty good post about DBC <a href="http://www.lostechies.com/blogs/sean_chambers/archive/2007/10/06/designbycontract-with-fluent-interfaces.aspx">here</a>, in which he implemented a fluent inteface to enforce this.

So instead of:
if(obj == null)
throw new ArgumentNullException("Cant be null")

You can say
Check.Parameter(obj).IsNotNull().FailWithMessage("Can't be null") or
Check.Parameter(myString).IsNotNullOrEmpty().WhenTrimmed().FailWithMessage("xxxxxx")

The benefit (to me at least) besides the sweet gooey symantic sugar is that DBC is encapuslated, and if you need to you can turn it off.


Tuesday, 16 October 2007 09:37:06 (Pacific Daylight Time, UTC-07:00)
I like the fluent interface idea. I hadn't seen that before.
Thanks, Joe.
Friday, 19 October 2007 21:36:35 (Pacific Daylight Time, UTC-07:00)
The example doesn't give quite enough information to properly answer the question.

First of all, it's not so much where it's called from, but by whom. If "obj" is coming from a user, or somewhere else out of your control, then clearly you'd have to verify the parameter, because you just can't trust them. On the other hand, if it's just being called by another process in your system, then it's not the callee that breaking the contract, but the caller --- It needs to be fixed, but then, once it is, your done with it. In this case, a Debug.Assert() would be more called for.
This is important -- In your example, the parameter check triples the complexity of the method --- This may affect the JITter ability to inline the function.

I might go for two functions, an internal one with no checking, and a public one which checks the parameters, and then calls the other.

Then we have the question of exactly what should be checked. In C#, where we know that "obj" is either valid SomeObject object or null, then the null check may be fine. But if this were say a C or C++ method:
int DoThingy(SomeObject* pObj)
{
....
}

where pObj could be anything, checking for just one specific bad value is pretty much a waste of time (i.e., calling DoThingy((SomeObject*)1); is just as wrong, but will nevertheless fly right through your paramter check.


Tuesday, 30 October 2007 15:03:34 (Pacific Standard Time, UTC-08:00)
Something that might be worth checking out is nvalidate.org the idea is similar to the syntactic sugar that Joe sites, it's just an easy way to consistently check state.

I always viewed it as paranoid coding to check every parameter. But I'm a paranoid coder so I like it. My rule has always been, if I can (read: will a customer let me write code the way I want too) I check all required parameters of any public method, because in the end I have no real control over the code that will call it. This both can provide faster debugging, and when used with a sugary validation syntax, a more explicit code base.
Comments are closed.