Advertisements
Posted by: jsonmez | April 16, 2012

Validate User Input, Not Developer Input

I have a very simple rule, I like to follow that helps to simplify my code.

“Don’t validate developer input”

This rule simply means that we should not try and validate input that came from a source that is not a user or external system.

Another way to put it would be, don’t validate parameters you pass around in your own code.

γνῶθι σεαυτόν

(Know thyself)

knowthyself

While you can’t guarantee input from another source is valid, you can know that your own code’s input is valid.

When writing a method, we should really strive to know who is calling that method and put the onus on the caller of the method not to pass in junk.

I know this isn’t a popular opinion, because it seems to go against the idea of “defensive programming,” but the idea of defensive programming is misunderstood.

Chuck Norris himself once said:

They say the best defense is not to offend.

defense

That slogan applies well here.  Your strategy should be to make sure you always pass valid values into methods rather than trying to code methods to defensively do things with bad input.

(Let’s not be uncivilized!)

Let me give you an example:

var milkShakeMaker = new MilkShakeMaker();
var shake = milkShakeMaker.MakeShake(icecubes, milk, ingredients);

public class MileShakeMaker
{
   public Shake MakeShake(IEnumerable iceCubes,
             IMilk milk, 
             IEnumerable ingredients)
   {
      if(ice == null || milk == null || ingredient == null)
          throw new InvalidArgumentException("You passed in a null, duh");
      if(ice.Count < 1)
          throw new InvalidArgumentException("You need ice.");
      if(milk.IsSpoiled)
          throw new SpoiledMilkException();
      foreach(var ice in iceCubes)
      {
          if(ice.IsMelted())
             throw new MeltedIceException();
      }

     if(ingredients.Count < 1)
         throw new InvalidArgumentException("You need some ingredients.");

     ...

   }
}

 

Doesn’t that code look silly?

This is the exact opposite of what I am suggesting.

Rather than trying to decide which of these checks we should keep, and trying to follow some complex set of rules of when should we validate input like this, I am merely suggesting we get rid of all of it!

I’ve written about not checking nulls before, but I am convinced now that checking any input from code you control is a code smell.

What can you do?

Throw an exception at best, at worst, infer what was meant by the caller and corrupt data.

What purpose does throwing an exception there serve?  If you try and dereference a null object, you get an exception anyway.

Are you even going to catch an exception if you throw it?

If so, does that mean you are wrapping every method call you make in a try / catch block?

The big problem with trying to validate input that was passed to you by code you control is that you can’t really do anything useful.  You are just cluttering up your code.

Another question, if you are still gung-ho on validating input on all method calls… Do you even do it?

Do you actually check every parameter, or just when you remember.  Because, if you aren’t applying the checks uniformly, you cannot count on them.

Context

One major reason why I advocate validating what you pass into a method rather than what is passed into a method, is context.

If you are inside a method and someone passes you a null, you have no context.

What was going on that cause this parameter to be null?  I don’t know—I can’t know!

But—on the other hand, if I am about to call a method and I am about to pass that method a null, I know why.  I know that the list I got back was null and I could check for that and instead pass in an empty list to the method I was about to call.

Better yet, I can take it one more step further and not ever pass null back from the method that returns the list in the first place.

The point is that you have way more context before you call the method, than you do when you are in the method.  If you are going to validate the input, do it in a place where context will allow you to do something meaningful, rather than just throwing a random exception.

Better code

Thinking this way will force you to write better code.  I firmly believe it. 

Most of us don’t really think about what we are passing into methods or whether or we are returning nulls from methods, we instead tend to think about what we are being passed.

By changing things around, it leads us into better coding practices that are less error prone.

We are forced to think about standardizing and restricting ranges of values for parameters.

We are forced to start considering making our objects immutable, so that if we properly initialize them, they cannot contain null values.

Most importantly, our code becomes cleaner, because it isn’t littered with error checking.  Instead of checking for errors everywhere, we are coding in a way that prevents them from being possible.

So the next time you are thinking about validating parameters passed into your method, by another method you have control over, don’t do it!

If you can’t trust yourself, who can you trust?  (Chuck Norris perhaps?)

Advertisements

Responses

  1. I couldn’t agree more 🙂

  2. That’s why I like Code Contracts. I leave “full checking” on in Debug builds, so when I’m developing I get all the usage exceptions (and the debugger gives you the context in that situation).

    For release, you can choose between two options:
    1) Turn on Preconditions on Public APIs Only, so other developers do get usage exceptions (which the .NET BCL, for better or worse, has trained .NET developers to expect). All private/internal methods don’t get checked.
    2) Turn off all checking and ship a Contracts dll. This enables other developers to “opt in” to preconditions on your dll (e.g., during development) and then have a release version with no checks at all.

    I wrote a simple blog entry on getting started with code contracts (assuming option (2)):
    http://nitoprograms.blogspot.com/2011/01/simple-and-easy-code-contracts.html

    • That is a good way to go also, thanks for the link.

  3. Great Post,

    I had a similar discussion with a coo-worker about how it is important to validate and transform the data from the UI layer or closest to the origin so that the rest of our code can use the actual type through out (for example don’t pass a string through all your layers if its an int), thus preventing the validation and parsing else where. A bit different then your post but I believe the values are the same.

    Thanks for the good read.

    • Yeah, pretty similar idea. I also like to encapsulate input into classes the further restrict the contract of what can be done with the object. For example, I might create a specific Date class in my application called MyApplicaitonDate that I can tightly control, by making my methods require it and parsing user input directly into that type as close to the UI layer as possible.

  4. “What purpose does throwing an exception there serve? If you try and dereference a null object, you get an exception anyway.”

    Perhaps it’s a matter of convention, but if I’m using some API written by someone else on the project, the nature of that exception communicates something to me. If I get a “NullReferenceException”, I have no way of knowing the intent of the method writer. It might be that I’m passing an invalid argument to the method, or it might be that the other programmer made some rookie mistake in his code that has nothing to do with me. (like calling a method that returned null and then passing the null value to some other method).

    If, on the other hand, I get an ArgumentException or some inheritor thereof, the other developer is saying “hey, you’re doing it wrong, buddy.” In the former case, I have to go read through his code to figure out what the problem is. In the latter case, I don’t.

    On the whole though, I agree that the boilerplate in the milkshake making method is absurd and that better solutions exist. I’m just not sure I’m fully on board with putting the understanding burden on client callers, even when those callers are me. It seems conceptually to violate information hiding. But, it does keep the code more readable and it does fail early, so maybe I should try it on for a while.

  5. […] The improvements include generating CAML queries using LINQ to XML, taking a more functional approach to mapping from loosely typed to strongly typed data, passing in more specific arguments, not tying the repository to a definition class, and removing assertions to focus on validating user input, and not developer input. […]


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Categories

%d bloggers like this: