How should I handle invalid user input?

Question

I have been thinking about this issue for a while and I would be curious to have opinions from other developers.

I tend to have a very defensive style of programming. My typical block or method looks like this:

T foo(par1, par2, par3, ...)
{
    // Check that all parameters are correct, return undefined (null)
    // or throw exception if this is not the case.

    // Compute and (possibly) return result.
}

Also, during the computation, I check all pointers before dereferencing them. My idea is that, if there is some bug and some NULL pointer should appear somewhere, my program should handle this nicely and simply refuse to continue the computation. Of course it can notify of the problem with an error message in the log or some other mechanism.

To put it in a more abstract way, my approach is

if all input is OK --> compute result
else               --> do not compute result, notify problem

Other developers, among them some colleagues of mine, use another strategy. E.g., they do not check pointers. They assume that a piece of code should be given correct input and it should not be responsible for what happens if the input is wrong. Also, if a NULL pointer exception crashes the program, a bug will be found more easily during testing and have more chances of being fixed.

My answer to this is normally: but what if the bug is not found during testing and appears when the product is already being used by the customer? What is a preferred way for the bug to manifest itself? Should it be a program that does not perform a certain action, but can still continue to work, or a program that crashes and needs to be restarted?

Summarizing

Which of the two approaches to handling wrong input would you advise?

Inconsistent input --> no action + notification

or

Inconsistent input --> undefined behaviour or crash

Edit

Thanks for the answers and suggestions. I am a fan of design by contract too. But even if I trust the person who has written the code calling my methods (maybe it is myself), there can still be bugs, leading to wrong input. So my approach is to never assume a method is passed correct input.

Also, I would use a mechanism to catch the problem and notify about it. On a development system, it would e.g. open a dialog to notify the user. In a production system it would just write some information to the log. I do not think that extra checks can lead to performance problems. I am not sure if assertions are enough, if they are switched off in a production system: maybe some situation will occur in production that had not occured during testing.

Anyway, I was really surprised that many people follow the opposite approach: they let the application crash "on-purpose" because they maintain that this will make it easier to find bugs during testing.

Answer 1

You've got it right. Be paranoid. Don't trust other code, even if it's your own code. You forget things, you make changes, code evolves. Don't trust outside code.

A good point was made above: what if the inputs are invalid but the program does not crash? Then you get garbage in the database and errors down the line.

When asked for a number (e.g. price in dollars or number of units) I like to enter "1e9" and see what the code does. It can happen.

Four decades ago, getting my B.S. in Computer Science from U.C.Berkeley, we were told that a good program is 50% error handling. Be paranoid.

Answer 2

You already have the right idea

Which of the two approaches to handling wrong input would you advise?

Inconsistent input --> no action + notification

or better

Inconsistent input --> appropriately handled action

You can't really take a cookie cutter approach to programming (you could) but you'd end up with a formulaic design which does things out of habit rather than from conscious choice.

Temper dogmatism with pragmatism.

Steve McConnell said it best

Steve McConnell pretty much wrote the book (Code Complete) on defensive programming and this was one of the methods that he advised that you should always validate your inputs.

I don't recall if Steve mentioned this, however you should consider doing this for non-private methods and functions, and only others where deemed necessary.

Answer 3

There's no "correct" answer here, particularly without specifying the language, the type of code, and the type of product that the code might go into. Consider:

• Language matters. In Objective-C, it's often okay to send messages to nil; nothing happens, but the program doesn't crash, either. Java doesn't have explicit pointers, so nil pointers aren't a big concern there. In C, you need to be a little more careful.

• To be paranoid is to unreasonable, unwarranted suspicion or mistrust. That's probably not any better for software than it is for people.

• Your level of concern should be commensurate with the level of risk in the code and the probable difficulty of identifying any problems that do show up. What happens in the worst case? The user restarts the program and continues where they left off? The company loses millions of dollars?

• You can't always identify bad input. You can religiously compare your pointers to nil, but that only catches one out of 2^32 possible values, nearly all of which are bad.

• There are a lot of different mechanisms for dealing with errors. Again, it depends to some degree on the language. You can use assert macros, conditional statements, unit tests, exception handling, careful design, and other techniques. None of them is foolproof, and none is appropriate for every situation.

So, it mostly boils down to where you want to put responsibility. If you're writing a library for use by others, you probably want to be as careful as you reasonably can about the inputs you get, and do your best to emit helpful errors when possible. In your own private functions and methods, you might use asserts to catch silly mistakes but otherwise put responsibility on the caller (that's you) to not pass garbage in.

Answer 4

There should definitely be a notification, such as a thrown exception. It serves as a heads up to other coders who may be misusing code you wrote (trying to use it for something it wasn't intended to do) that their input is invalid or results in errors. This is very useful in tracking down errors, whereas if you simply return null, their code will continue along until they try to use the result and get an exception from different code.

If your code encounters an error during a call to some other code (perhaps a failed database update) which is beyond the scope of that particular piece of code, you really have no control over it and your only recourse is to throw an exception explaining what you know (only what you are told by the code you've called). If you know that certain inputs will inevitably lead to such a result, you can just not bother executing your code and throw an exception stating which input is not valid and why.

On a more end-user related note, it is best to return something descriptive yet simple so that anyone can understand it. If your client calls and says "the program crashed, fix it", you have a lot of work on your hands tracking down what went wrong and why, and hoping you can reproduce the problem. Using proper handling of exceptions can not only prevent a crash, but provide valuable information. A call from a client saying "The program is giving me an error. It says 'X Y Z is not valid input for method M, because Z is too large", or some such thing, even if they have no idea what it means, you know exactly where to look. Additionally, depending on your/your company's business practices, it might not even be you fixing these problems, so it is best to leave them a good map.

So the short version of my answer is that your first option is the best.

Inconsistent input -> no action + notify caller

Answer 5

I struggled with this same issue while going through a university class in programming. I leaned towards the paranoid side and tend to check everything but was told that this was misguided behavior.

We were being taught "Design by contract". Emphasis is that the preconditions, invariants and post-conditions be specified in the comments and design documents. As the person implementing my part of the code, I should have trust in the software architect and empower them by following the specifications which would include the preconditions (what inputs my methods have to be able to handle and what inputs I will not be sent). Excessive checking in every method call results in bloat.

Assertions should be used during build iterations to verify program correctness (validation of preconditions, invariants, post conditions). Assertions would then be turned of in the production compile.

Answer 6

Using "assertions" is the way to go to notify fellow developers that they are doing it wrong, in "private" methods only of course. Enabling/disabling them is just one flag to add/remove at compile time and as such it is easy to remove assertions from the production code. There are also a great tool to know if you are somehow doing it wrong in your own methods.

As for verifying input parameters within public/protected methods, I prefer to work defensively and check parameters and throw InvalidArgumentException or the like. That is why there are here for. It also depends on whether you are writing an API or not. If it's an API, and even more if it is closed source, better validate everything so that the developers know precisely what went wrong. Otherwise, if the source is available to other developers, it is not black/white. Just be consistent with your choices.

Edit: just to add that if you look for example at the Oracle JDK, you will see that they never check for "null" and let the code crash. Since it is going to throw a NullPointerException anyway, why bother checking for null and throwing an explicit exception. I guess it makes some sense.