Re: Offensive Coding
Posted: Jul 17, 2006 9:01 AM
> The problem with the, "I'll rescue you" programming is
> that there is no way to know what the caller meant when
> they passed in the bad parameter. In the flight control
> system example, maybe the null parameter meant 'empty'
> maybe it's a bug and treating it as empty will cause the
> program to fail silently and the plane to crash, whereas
> blowing up will let the pilot know that the flight-system
> has failed. More importantly, silent failures are a lot
> harder to detect during testing and I'm pretty sure flight
> control systems are tested pretty well.
> My experience is that a lot of bugs that make it into
> production are the result of not blowing up when things go
> wrong. For example, I had a bad day where I had to
> manually merge some code with a contractor's (don't ask)
> and I screwed up. I didn't notice the issue in limited
> testing (because I stupdily decided it was already
> tested.) What happened next was extremely ugly. I was
> passing in bad data, instead of failing up front and
> rejecting the input, it placed orders in the system with
> corrupted data. It took quite a while for the data to
> percolate through the system and make the problem
> apparent, so meanwhile tens of thousands of bad orders
> were being placed.
> If the code had just blown up on bad input 1. I wouldn't
> have put it into production and 2. even if I had, the
> problems would have been manifested immediately with
> failed orders resulting in a short downtime instead of a
> huge debacle.
I'm not interested in rescuing anybody else. I'm interested in my program acting correctly on any input. There should rarely, if ever, be any 'maybe' about what is passed in. You should be able to fail to a reasonable state when you get bad input. You should be able to figure out if the input you are getting is good or not and you should be able to figure out what to do in either case. The amount of processing you do in this regard is totally dependent on the system you are writing. I agree that failing silently is bad. I would further say that, if you are concerned with failing silently then your time would be better spent getting rid of all those empty try/catch blocks because of checked exceptions than they would be looking for spurious null checks.
While your story is bad (This makes a much better case than the original post to me, but lacks sweeping generalizations about spurious null checks), from the customer's standpoint it probably did the "right" thing. Bad orders, while painful to deal with, can be fixed. If I was a customer and I was inputting an order into an order entry system and I got something that said "Null Pointer Exception" I would immediately go on to use your competitor's product and probably never return to yours. If I ordered "Harry Potter and the Chamber of Secrets" and I recieved "Potting Plants by Harry Chambers" I would call up the seller, tell them they sent me the wrong book, get it corrected and then go on my way. In your case, if a lot of users happened to have failed orders in that short downtime, your users would likely be shopping for or with a competitor's product.
The real problem with the example in the original post is not that it checks for null but that it doesn't do anything if it detects the bad case. Instead of going on to do nothing (I guess that makes it a spurious null check) it should let you know in a graceful manner what went wrong. This is where I guess I totally disagree with the Michael's post. He goes on to explain all the ways to just get rid of the null check and says that you have to trust the input because you are in a stupid free zone. Problem is, there are not stupid free zones. Even if we're not stupid we sometimes make mistakes. You don't sound stupid to me but you have a brilliant example of something that can be called stupid.
Assuming you will never get bad input is, in my experience, a huge risk. That's like saying you should drive without a seatbelt. Hey, I've had no accidents. I'm a totally competent driver. I never speed. I always use my turn signal. I have never, in my life, broken one traffic rule. Guess what, that seat belt isn't there because you don't know how to drive. It's there because out there somewhere is some person who doesn't. Or, more to the point, he doesn't drive like you. I think programming is like that in a lot of ways. People that don't program the way you do are wrong or stupid or pick your favorite adjective. I know quite a few people that would read Michael Feather's article and say
"WOOHOO, I don't need to worry about bad input. I'll just put in little comments in my code saying
// Stupid-Free zone. Do not pass in any bad input
and then get back to reading Artima."
The real problem, as you point out, is failing silently. And I don't think that you need to worry much about the caller's intent. If your code is truly encapsulated it should have an idea about what constitutes good input and bad input and then act accordingly. And the user should be informed in some manner that leads to a way to resolve the issue.