Redundant validation is evil

Don’t you hate it when something you think of as a good habit turns out to be a bad habit? I have long been a big proponent of defensive programming. Never assume the arguments being passed to a method are good, always validate everything, and generally protect yourself from other people’s bad code or even your own bad code elsewhere in the system. I’m here to tell you that you can go too far with this philosophy.

The systems design principle “don’t repeat yourself” is often thought of in terms of copy and paste. If you need to do the same thing in two different places, don’t copy the code from one place to another, instead put it in a method and call it from both places so that if the process changes you don’t have to remember to update the logic in more than one place.

A more subtle way to violate this principle and really get yourself tied in knots is to do the same validation in more than one place. Here’s an example of what I’m talking about. You have two classes, one produces widgets, and puts them in a collection. The other takes those widgets and presents them to the user, perhaps on a Web page.

If you’re an MVC person, think of the code that produces the widgets as being in the model, and the code that presents the widgets being in the view. Further assume that the widget-producing code is expected to produce three widgets at a time. You’re writing the code for your view layer, and you want to make sure your code isn’t going to blow up due to bad input.

The first check is a null check. You don’t want to get a NullPointerException that gets passed back to the user. (In Ruby, you need to do a nil check to avoid a NoMethodError.) That’s straight defensive programming, and needs to be kept around.

Depending on how your page works, you may want to make sure that the collection isn’t empty. So you write something like:

if (null != widgets && !widgets.isEmpty()) { // do stuff }

That’s perfectly sensible. You expect there to be three widgets in the collection, so why not also do this?

if (null != widgets && !widgets.isEmpty() && 3 == widgets.size()) { // do stuff }

Here’s where the problems start. First of all, that 3 should be a constant that’s defined elsewhere so that if 3 changes to 4 or 5 or 2, this code gets updated along with everything else. But the truth is that you should leave out that expression entirely. It looks like basic defensive programming, but it’s really business logic. You just codified the “you must have 3 widgets rule” right there in your view layer. When requirements change and the number of widgets becomes variable, this is the piece of code that will keep you up late at night.

Either this is the only place where you check the number of widgets, which is bad because it’s separated from the code that generates the widgets by a great distance, or you’ve just repeated yourself, and when you update your widget generating code, you’ll forget to update this little bit of logic in the view layer and you won’t able to figure out why your new code is generating two widgets just as you’d expect, but they’re not showing up on your Web page.