When Do You Check For Bad Arguments
When Do You Check For Bad ArgumentsContext: You're writing an object to perform some task. There are methods which take arguments that could, in some way, be bad.
Problem: How do you check for bad argument values? For example, you might have a Person object and a method (Java syntax) that has the following partial declaration
public --- setHeight(int height);
Now, it turns out that you want height to always be a positive integer. The problems are: how do you enforce this?
Forces:
Efficiency. You don't want to repeatedly check the same value. Otherwise the code can bog down.
Code maintenance. Multiple validation points in the code could easily get out of sync and cause code maintenance problems
Readability. Checking that arguments are valid clutters code and makes it harder to follow the "real processing" in a method. This is also the case if the client code needs to check boolean flags ("did the set operation succeed").
Loose coupling. You'd like your objects to be as independent as possible. The object implementing setHeight shouldn't have to depend on the kindness of other arguments and hope they pass in the correct values.
Early detection. Ideally, you'd like to spot bogus input as soon as possible. Suppose a chunk of data comes over the wire (or from the user). If it's part of a single transaction, you'd like to validate all of it and then process all of it. Or suppose the user is entering data. Immediate feedback ("that value is invalid") can be quite helpful.
Solutions:
There are many possible solutions. Among them are:
Check the value at the set method
The argument for placing the check here involves the semantics of set. Set indicates that some piece of state is being set. Which is where validation should go. This leaves open the question of how to signal a failure (by return value or via exception).
Define a validation method / method object
This helps solve the Early Detection problem. If the constraints are reified into explicit public methods or objects, then they can be called from other locations in the code. The constraint is only implemented once (helping with code maintenance).
The downside here is that this leads to clumsy interfaces
public --- setHeight(int height);
public boolean isHeightValid(int height);
and can lead to inefficient execution (the constraint may be checked more than once). Moreover, just from the above interface, it's not clear whether the client is supposed to call isHeightValid before calling set.
Related Patterns:
OnceAndOnlyOnce, surely there is something on wiki about attaching validators to text fields, the entire checks pattern language, ExceptionPatterns.
Most of the discussion below concerns what to do about bad arguments and _whether_ you check for them (my approach to that is to only validate arguments that will break something if wrong - if the system can handle negative arguments, then allow them - that sort of validation is best handled as a business rule or something). The real question is _when_ to check an argument. When the argument is receieved? Or when it is used? What if, as in the example below, the argument is going to be stored in a field and then used later? Should it be OnceAndOnlyOnce? Or can the autoMechanic assume that the Car has four tires, since the Car will not allow any other number of tires? Within a function (or chain of functions in properly-factored-code) should the outer functions that take arguments in order to pass them on to inner functions validate them, or allow the inner functions to do the validating? Consider how some arguments may not actually be used - if the Logger object is null, does it matter if nothing is logged in the execution paths where it is given as null?
I find something like the following to be a good solution (using the example above):
public class Height implements/extends ----- {
public Height( int height ) throws InvalidHeight? {
// Check parameter, throw if not good.
// Store state
}
}
Now the method signature on Person would look like
public ---- setHeight( Height height )
I suppose the setHeight method would need to check that the parameter is not null and behave accordingly.
I prefer lots of small objects. This is probably the reason I prefer this solution.
I prefer never checking arguments. I'd make exceptions for arguments that could make me overwrite memory, kill the patient, crash the plane. So for the example given, the question is, What would happen if a negative argument is passed. If the rectangle gets drawn upside-down, I'd ignore it.
Again in the example given, rather than complain, I'd be more likely to force the height non-negative via something like
setHeight: anInteger height := anInteger absBetter yet, I'd just accept the arg. The real place to validate such a value is in the input routines, on the form that contains the field, or in the file reader.
My mum always told me when you find a bug, report it so that it can be fixed, don't just hide it.
The style in the example above (forcing an integer with abs) works for me only when correctness of the results is secondary to keeping the system running. -- MichaelFeathers
Well, yes, but of course if you don't mind the system not running, there's no point in checking anything.
But seriously, to you and the loving mother above, when I use software, I want these things:
Test, before shipping, for everything you can. Protect against damage and crashes. Stay out of my face. That's what I ask from programs I pay for.
And remember ... your error-handling code is almost certainly the least-tested part of your system. The less of it you have, the better.
It really depends on the domain. In some domains, correctness of results or actions is paramount. Fudging data rather than reporting would not only be unethical, but also could be illegal. In other domains the system should never go down. Shrinkwrap software, telephone switches, medical research systems and missile guidance systems don't really lump together well with regard to their error handling policies.
I use assertions for this kind of thing. Either:
void setHeight( int height ) {
Assert.assert( height >= 0 );
this.height = height;
}
or:
void setHeight( int height ) {
this.height = height;
assertValid(); // assertValid() will check the new height.
}
-- DaveHarris
No, but seriously, gentlemen, if the bloody thing is invalid, what do you plan to do? And under what circumstances need it be different from what you'll do when an unknown, unplanned error occurs? I've only been doing this for 35 years or so, but I've never seen a situation where inserting lots of assertions actually made the program better.
If you're processing input and there is a human there, sure, check it, make him type his height without the minus sign. If you're processing an input file, sure, write a diagnostic and ignore the record.
But if the system is running and suddenly sets the height of some random individual to -4, what do you do?
The original question here was WHEN do you check for bad arguments, which seems to me to be interesting. My answer to that would be:
WHEN the result of not checking is substantially worse than letting the system do whatever it will do with the bad value. If it's a personnel system, the world will not end if some guy's height is recorded as negative. If it's an X-ray dosage calculator, it might be best to get to the shutdown routine ASAP. If it's the flight controls for an F-117, you don't dare shut down, the plane won't fly without the computer. The height value of "underground" might be incorrect, but if you shut down, it won't be incorrect for long.
WHEN the value is being input. If a human is there, prompt him. If it's an input file, ignore the input and file an error message.
NOTWHEN some random routine deep in the system is being called. Write most code to assume that its arguments are correct, and to ensure that the values it uses to call other methods are correct if its inputs are. This will result in less code. Since number of errors is directly proportional to (other things being equal) code size, error-checking and correcting code increases the probability of error and needs to be used as rarely as possible.
-- RonJeffries
Continuing to look for a simple answer will just lead us around in circles. A similar discussion a few years ago led me to write the ChecksPatternLanguage, which devotes only one or two of its ten patterns to this issue. There must be more. Let's look for the patterns I missed. -- WardCunningham
Possibly the most important clue leading us to avoid oversimplifying is the example given of an HR system, a medical treatment system, and an aircraft control system. Here the lessons of DesignByContract are applicable. In the case of the widely-cited ArianeFive failure, a contract that explicitly stated the required valid inputs and the consequences of invalid out-of-range inputs could have been an important element in preventing that accident - although not the only one. Suggested additional pattern:
I prefer GNOME's glib style such as:
const char* strcpy (char *dest, const char *src) {
g_return_val_if_fail (dest);
g_return_val_if_fail (src);
while (*src)
{
*dest = *src;
dest++; src++;
}
}
g_return_*_if_fail functions return if the condition failed as the name implies. Unlike assert, they work both at debug-mode and release-mode. Unlike usual return, they left a log or trace message, so they don't hide bugs.
When should we check arguments? The answer is obvious, always. It's not the responsibility of functions to continue to work with wrong arguments.
At the interfaces to external systems, I assume that everything that could possibly go wrong will. (I consider the user to be an "external system." ;-) So, at these interfaces, I make the code be as paranoid as possible: No matter how well defined and widely accepted the industry standard defining the interface is, I assume that the other party will mess it up, in some creative new way. This approach has worked quite well for me. (I've yet to see an ANSI.X12 implementation that actually conforms to the standard. And don't ask me about Microsoft's implementation of their own "Simple Mail API"!!! ;-)
Now deep inside a program... Well, how much do you trust your peers?
Validating all parameters all the time is costly; you have to write the code, and it inhibits refactoring. When it reports a problem (typically of the null pointer variety) it's helpful, as it usually finds the problem closer to the cause than would otherwise be the case. But this advantage is moderated by the cost, and the presumed presence of good automated test coverage, via TestDrivenDevelopment, for example.
So I generally recommend that people find the "boundaries" with the "outside," and focus the validation efforts there. This seems to be a more effective approach than putting validation in every conceivable location.
-- JeffGrigg
Function GetDayOfWeek (Date)
begin
Assert (Date is a date)
Assert (One argument passed)
Assert (User speaks English)
Return 'Monday,Tuesday,Wednesday,Thursday,Friday,Saturday,Sunday'[GetRemainder(Date / 7) + 1]
end
Yeah, I guess validating every argument works for one, so it should work for many. I like the way these assertions do not clutter the code.
In order not to violate OnceAndOnlyOnce, I often to test the validity of the data only in the lowest level of routines, where the errornous values really have an impact on the execution. As an example consider a font rendering engine, and the "user" requests rendering of a -2 pt heigh "A" letter. The outer routine could then pass the height parameter unmodified into the internals of the system, where finally some allocateGlyphBuffer(width, height) could complain about negative heights.
Pros:
EditText of this page
(last edited October 17, 2006)
or FindPage with title or text search