Public method argument validation

[Frank Harper]

I want to ensure that public methods are called with valid arguments.

I'm looking for a good systematic strategy for implementing the checks in all my methods.

For example, suppose I there is method of with an argument arg of type String that must be non-null and have visible content. The strategies that have occured to me are:

1 - Inline argument validation

public void aMethod(String arg) {
       if (arg == null)
        {
            throw new NullPointerException("Arg must be non-null.");
        }
        if (arg.matches("\\s*"))
        {
            throw new IllegalArgumentException("Arg must have visible content.");
        }
   ...
}

Advantage : It's very simple!
Disadvantages : It's 8 lines of code. I might have a method with 3
arguments that need the same validation.
So I'll end up with a lot
of validation code at the beginning of many methods.

2 - Refactor into generic validation method

public void aMethod(String arg1, String Arg2, String Arg3) {
    Util.validateIsVisible(arg1,"arg1");
    Util.validateIsVisible(arg2,"arg2");
    Util.validateIsVisible(arg3,"arg3");
   ...
}
 
public class Util
    public boolean validateIsVisible(String arg, String argName)
    {
       if (arg == null)
        {
            throw new NullPointerException(argName + " must be non-null.");
        }
        if (arg.matches("\\s*"))
        {
            throw new IllegalArgumentException(argName + "must have visible content.");
         // TODO check that argName isn't null.
        }

Advantage : The validation code isn't duplicated.
Disadvantages : The extra added argName arguments are ugly.

3 - Use reflection to get rid of extra argument argName argument in previous solution

String m_arg;
public void aMethod(String arg) {
     m_arg = arg;
     validateIsVisible(this.getClass().getField("m_code"));
   ...
}

Advantage : None!
Disadvantages : Ugly complicated reflection code all over the place.
Doesn't work with non member arguments.

So far it looks to me like solution #2 refactor into generic validation method method is the best.

Thanks
Frank

[Tim West]

Another option as of JDK1.4 - use assertions:

public void aMethod(String arg1, String arg2, String arg3) {
   assert(! StringUtil.nullOrEmpty(arg1)); // Assuming you have such a class...don't we all?
   assert(! StringUtil.nullOrEmpty(arg2));
   assert(StringUtil.isVisibleContent(arg3));
 
   ...
}

Advantages: throws a nice AssertionError when failed. Short.

Disadvantages: requires JDK1.4 or better.

I think it's legit to put the code that checks for particular String properties in one location, and my preference is for a "StringUtil" class (with all static methods). Using assertions rather than generic code in each class is much nicer for actually doing the checking.

As an alternative, there is a school of thought that says "never return null, never pass null". It seems like you're part way there . Also, you may want to look at the "Null Object Pattern".

Cheers,


--Tim

[Stan James]

One of the guys on my team extracted the validator classes from Struts to use in POJO. It's got a learning curve, a bunch of little classes and XML files, so I doubt anybody would call it real simple, but it does a neat job of making reusable validations, messages, etc.

[sever oon]

There's another popular approach to this sort of problem you may wish to consider. Formally, I'm talking about Design by Contract (DbC), but I do not often write code to full DbC standards.

DbC simply says that you should associate a "contract" with each method you write (actually, the contract is associated with the class you're writing, to be a stickler about it). If your method can accept any arguments at all, then no matter what the user passes in, you should hand back a return value that is defined in the documentation. Example: if arg1==null, this method returns "x".

As you can imagine, it's very difficult to write a system where every method always results in a defined return value, because often a developer expects only a certain range of inputs that doesn't cover the entire space implied by the argument type (just because my method takes a String doesn't mean it will deal with every possible String). So, DbC provides for this by allowing designers to put "preconditions" on callers. A precondition simply says, in order to call this method, the following condition must be met. Example: arg1!=null. (If a caller meets the preconditions of a particular method, any changes made to the state of the system are advertised in the documentation as "postconditions"...meaning that if properly called, you'll state the effects on the system of calling the method and the what return value to expect.)

What does DbC say about cases where the caller meets the preconditions, but for some reason the postconditions specified in the contract still cannot be met? Say, for instance, that the caller calls your method with legal values but there was some other problem in the system that caused the database to get nuked, and your method's successful completion is dependent upon that. This is where exceptions come in. This is known as an "exceptional situation"--the caller did everything they're supposed to do, and yet the method still cannot meet its end of the bargain. So, it throws an exception instead.

What about when the caller doesn't meet the preconditions? Then what is your method supposed to do? DbC would say that your method would have undefined behavior. Undefined means undefined...woe be to the caller that calls a DbC method without verifying that the preconditions were met. An exception *might* result, but so might an inconsistent state of the system (the database, for example, might get nuked). Or system.exit(0). Or do { int x=0; x++; x--; } while(true);. Or any other nasty thing you can think of.

It seems in your case that your method is throwing exceptions if the caller passes in invalid arguments. According to DbC, though, as long as you advertise the preconditions that callers are supposed to meet, you are under no obligation to do specific checking for those cases. You may, just to be nice, but callers have no right to expect or depend upon that functionality...you are perfectly free to remove those checks in the future and are under no obligation to maintain that behavior moving forward. You are only obliged to support your class' contract moving forward.

So, as this approach drastically simplifies your problem, I would use it! Decide what inputs you want your method to accept. If you truly do want to be able to handle every possible argument from callers, including nulls, etc, then your method should not throw an exception at all for those cases, but instead respond in a predictable fashion (return a defined value, for instance, like null). If you do not care to support those cases, then simply declare the preconditions callers must meet before passing args into your method, and from that point you may assume they are true without checking. (It doesn't hurt, by the way, to assert preconditions at the beginning of a method and postconditions just before returning--that's actually what asssertions were invented for...but keep in mind these are designed to be turned off at deployment time so no one should be depending upon them. They're simply debugging tools.)

One more thing that will get you started down the path to DbC. There's a way of setting certain ground rules that are always assumed to be true in DbC. For example, let's say that in the system you're writing you would like it to be assumed that there is always a precondition on every argument passed into every method that it be non-null. Rather than state this as a precondition everywhere, you can employ what are called "frame rules". A frame rule is simply a rule that applies to the class as a whole (or a package, or a subsystem, or a system, etc...they can operate over any level of granularity). So you can simply state a frame rule in your docs that make this a general rule for everything all the time, and override it in the individual cases where methods can handle nulls by specifying a precondition that explicitly allows null for a particular argument.

DbC is much more complicated than just what I've laid out here, and it can be used rigorously to mathematically prove APIs if you want to really get into it. But I rarely use more than I've described here in the workaday world.

Hope this helps!
sev

[Frank Harper]

First of all, Tim, Stan, Sever thanks for your thoughtful and thought provoking replies.

Concerning using assertions, here is what Sun has to say :

* Do not use assertions for argument checking in public methods.

Argument checking is typically part of the published specifications (or contract) of a method, and these specifications must be obeyed whether assertions are enabled or disabled. Another problem with using assertions for argument checking is that erroneous arguments should result in an appropriate runtime exception (such as IllegalArgumentException, IndexOutOfBoundsException, or NullPointerException). An assertion failure will not throw an appropriate exception.

As for using a DbC light style, that is what I was aiming for. In my code the contract is explicit : nulls are not allowed. But I also think there will be less bugs and that debugging will be easier if the contract is checked and a fast explicit failure results from a breach of contract.

There's actually a pretty good write up on this subject at Validate method arguments

It also addresses my original problem of simplifying the validation as much as possible.

[Tony Morris]

You might want to check out AOP for doing such a thing.
http://eclipse.org/aspectj

[Tim West]

I guess Sun and I disagree on this one. This often happens to me, usually because I favour more pure, theoretical things. I'd be the first to admit this is impractical, but it's how I think .

Anyway, let me clarify my thoughts on assertions. IMHO, they're the right thing to use for argument checking, under certain assumptions. Viz:

If the JavaDoc for a method says "This (String) argument should never be null", you're saying, effectively "behaviour is not guaranteed in this case". Therefore, an assertion ('assert(arg != null)') is legitimate. Sun says I should use an NPE instead...well, maybe. I'd ONLY do this if the JavaDoc explicitly said that an NPE is thrown.

OTOH, if the JavaDoc does not say this, then I agree, an assertion is absolutely the wrong thing to use. However, if the JavaDoc does not say this, I would say null should be accepted: acceptability of null is one thing, IMHO, that must be documented.

Sev, I use assertions in the implemenation of DbC. I'd say they work hand-in-hand. The contract for a method says (in part) "This param must not be null"; an assertion is used to enforce it. IRL, the fact that a contract exists on paper doesn't mean anything unless you enforce it...the same goes here. This is vastly better than saying "never return null", then doing something unspecified and undocumented. As you point out, you're free to remove the assertions in production code, and this is usually what happens. They exist purely to catch cases that should never happen in the first place.

(Specifying the contract of a method is not about returning a value for every conceivable input value, it's about specifying those that are correct, and what the response is in those circumstances. A contract places obligations on both parties involved. This isn't pure "design by contract", but it's practical and produces better code.)

The way I write code (when given the choice), anyone reading it can expect an AssertionError as soon as they violate the contract of the method: they know immediately when they do so. (Note that expecting it and relying on it are different things. If they catch the AssertionError, all bets are off: in production code you'd get badness). I think this is a Good Thing (TM). This and the no-nulls-ever rule make life a lot simpler I think.

Finally, a grossly OT remark: Eiffel does a MUCH better job of assertions than Java does... you get class invariants, loop invariants, formal precondition and postcondition declaration in methods...good things! Of course, you have to actually use them to get the benefit...

Anyway, those are my thoughts.



--Tim
[ June 08, 2004: Message edited by: Tim West ]

[sever oon]

Re: Frank's reply to my last post...

Sun is absolutely correct in their statement on assertions...callers should not be dependent on the fail-fast behavior they provide. Your statement, immediately following, on the other hand, I'm not so sure I can agree with:

I also think there will be less bugs and that debugging will be easier if the contract is checked and a fast explicit failure results from a breach of contract

If you really, truly believe that you can significantly affect the quality of your product in a positive way by fail-fast behavior, then build it into your contract. Let me give you an example...

I'm sure we're all familiar with the idea of a usage statement on command-line utilities. If I type in "dir /ljkl;akjl;das" at the Windows command prompt, I get a usage statement or an error message. Note that the dir program did not throw an exception, it did not do something unexpected...it behaves in a totally, 100% predictable way and instructs me on how to properly use the program. An instructional message, in other words, is part of the contract of the dir program. Restated a bit differently, the dir program accepts any input at all and has a "return value" associated with any of those possible inputs. Now, from a user standpoint, I might say that by printing out the usage message it's telling me that my input was "invalid". For my purposes as a user, of course it was invalid input becuase it didn't get me the information I was ostensibly after (unless I actually wanted to see the usage message). From an API standpoint, the input I provided was a valid point in the "space of inputs" accepted by dir.

So what's the difference between supporting any input versus restricting inputs in the contract, but supporting them with a particular behavior (say, assertions that fail right away). The difference is huuuuge. The difference lay in understanding the fundamental point of contracts. The point of a contract is to let the caller know what they have a right to expect from you, the implementor. It clearly demarcates where your responsibility ends and your caller's responsibility picks up. The real value of contracts is NOT simply documenting what inputs you expect...the real value of contracts is in setting expectations and drawing boundaries of responsibility.

For example...I write a method that declares as a precondition arg1!=null. It so happens the way I've implemented my method, if a caller hands me arg1==null, I throw a NullPointerException. The caller may, after a while, figure out that all my methods behave this way, and become dependent on that functionality, catching NPEs in certain circumstances and interpreting them to mean that arg1==null. One day, however, I'm faced with new requirements from management that require me to change that behavior--my methods will no longer throw NPEs if callers pass in arg1==null, instead they'll give some random, invalid return data.

The callers that have learned to depend upon NPEs might be angry...they may have scattered throughout their code all these catch blocks that they now have to change. They may cry and whine and insist that it is my responsibility to make sure I don't change that behavior, forcing me to opt for a hack solution where an elegant one exists if only I'm allowed to change that functionality.

The question is, did I ever intend to support that functionality? And that's where the contract comes in. Since I wrote that precondition arg1!=null at the outset, I'm well within my rights to change how my method responds to arg1==null at any time and all those callers who were depending on that behavior can go suck a lemon. Of course, properly used, DbC will prevent this situation from even occurring in the first place--that's the point, really. And therein lies its value.

That's why I say that you ought to insist that your method's behavior, if preconditions are not met, is undefined. You should resist giving any other outward appearance or answer. Now, when it comes to coding up your method, you are free to do any old thing you want for those undefined inputs, and you ought to choose the path that will increase quality the most (say, for instance, failing out right away with an AssertionError). But I always keep a careful eye on whether callers are beginning to depend upon certain undocumented behavior, and then I change it right away for a time and force them to do things without depending on what I like to call "incidental behavior", pointing at my contract and insisting all the while I never said I'd support that behavior. Once they get back in line, I go right back to implementing the undefined behavior that will result in the highest quality project. I do this because I believe that the first and most important step to quality is hammering out who's responsible for what and then holding people to their stated responsibilities...only then can language features like asserts or stack traces do the project any good. Once the people creating the codebase are allowed to get out of control, no amount of debugging tools will save the quality of the project.

So: externally, declare what you plan to support from here on out, and periodically shake up the other degrees of freedom to keep people honest. Internally, handle your undefined states in a way that will increase quality the most.

To address a few of the points that were made in the ensuing posts...Tim writes:

...the fact that a contract exists on paper doesn't mean anything unless you enforce it...

I must disagree. Assuming you understand contracts the way I do, it makes a *world* of difference that the contract exists on paper whether you "enforce" it or not (I put "enforce" in quotes because I consider a contract enforced if it meets its declared postconditions when the caller meets the declared preconditions). Because when stuff blows up, you can quickly figure out who's job it is to fix it. If people are depending on undocumented functionality, it's their job. If they met your preconditions and things still aren't working, it's your job. This ensures that changes occur at the source of the problem, not as hacks in surrounding code that really isn't responsible for the functionality misbehaving...this is how modules initially begin to assume responsibility for other modules and functional creep happens. The thing I love about contracts is that they are not just a coding tool, they are a management tool that keeps people honest when used properly...and better yet, they allow you as a lowly coder to manage your fellow coders without having to wait around for Pointy-Haired Boss to do it.

Continuing with Tim's post:

Specifying the contract of a method is not about returning a value for every conceivable input value...

Right. I agree wholeheartedly. Specifying a contract is not about returning a value for every conceivable input. It is about specifying the return value for every valid (meaning, "meets the preconditions") input, while declining to specify anything at all for inputs falling outside those boundaries.

Onwards:

anyone reading it can expect an AssertionError as soon as they violate the contract of the method: they know immediately when they do so. (Note that expecting it and relying on it are different things. If they catch the AssertionError, all bets are off: in production code you'd get badness).

You've made my point more articulately than I did, I think. I am in vehement agreement with you, sir! "Expecting" in the above quote means that you, as a coder, can expect to see something happen. "Relying" means that not you, but your code, is expecting to see something happen. First case: fine. Second case: nope. (I might in some cases go so far as to even say that, in the first case, a given response to an invalid input should not even be expected. There might be a particular situation where it is extremely difficult or even impossible for me to fail out in a particular "expected" fashion. If a caller has no way of detecting that particular situation, they might be surprised when "the usual" doesn't happen, to which I say: "Ha ha! Gotcha!" And then I point and laugh...)

(Which usually gets me a , , and then a , by the way.)

sev
[ June 08, 2004: Message edited by: sever oon ]

[Frank Harper]

Thanks Sev for expanding on your point of view. I hadn't really understood why you wrote in your first post :

If you truly do want to be able to handle every possible argument from callers, including nulls, etc, then your method should not throw an exception at all for those cases, but instead respond in a predictable fashion (return a defined value, for instance, like null). If you do not care to support those cases, then simply declare the preconditions callers must meet before passing args into your method, and from that point you may assume they are true without checking. (It doesn't hurt, by the way, to assert preconditions at the beginning of a method and postconditions just before returning--that's actually what asssertions were invented for...but keep in mind these are designed to be turned off at deployment time so no one should be depending upon them.

Now it's completely clear since you wrote :

the real value of contracts is in setting expectations and drawing boundaries of responsibility

I took a look at Eiffel and it appears to have the same problem as Java. It also treats contract breaches as exceptions that can be caught.