How to consolidate validity checking and exception throwing in Java?

Question

I am implementing an interface which defines a method that can throw an exception if the parameters are not valid. What constitutes valid parameters depends on the implementing class. The interface also defines an isValid() method which can be used to check the parameters but returns a boolean rather than throwing an exception. I have found that implementing both methods will cause a lot of duplication. Consider this made up example:

public class Something implements SomeInterface {

    // Other class stuff

    public void doTheThing(SomeParameter sp) throws SpecificRuntimeException {
        if(sp == null) throw new ParameterCannotBeNullException();
        if(sp.getNonZeroInt() == 0) throw new ShouldBeNonZeroException();
        if(!sp.someOtherCondition()) throw new SomeConditionNotMetException();

        ...
    }

    public boolean isValid(SomeParameter sp) {
       if(sp == null) return false;
       if(sp.getNonZeroInt() == 0) return false;
       if(!sp.someOtherCondition()) return false;

       ...
       return true;
    }
}

The problem is the checks in both methods must be consistent, and are essentially duplicated logic. I've been trying to consolidate the checks so that both methods use the same checks but the behaviour is still retained. Some things I considered:

1. in doTheThing() have the line if(!isValid(sp) throw new RuntimeException();
2. separate the exception throwing part into a separate, private method, say checkParameter() and in isValid() do: try { checkParameter(sp); return true; } catch (SpecificRunTimeException e) { return false; }
   

The problem with 1. is that the specific exception is lost, and I want to provide as detailed an exception as possible. The problem with 2. is using the exception mechanism seems... wrong somehow. This part of the code may be performance sensitive, and I don't want to depend on something that's fundamentally slower than it needs to be (if I have to do it this way and profiling doesn't show a problem, fair enough... but what if is a problem?). Or has the performance hit of using exceptions this way been shown to be negligible?

What is the best way to refactor this code to use the same validity checking logic?

Answer 1

Your urge to "say it once and only once" is laudable. Good thinking; it'll be worth the effort.

Maybe you can create an abstract class that implements the interface and provides the default implementation for isValid().

If your object is mutable and has setters, perhaps you could move the check that throws IllegalArgumentException into each one and then call them. You'll have a more fine-grained view of what went wrong that way.

Answer 2

I don't know if it's the best way, but a way is to have an internal method like this:

private boolean isValidInternal(SomeParameter sp, boolean throwIfInvalid)
    throws SpecificRuntimeException

Then you call isValidInternal with true from the doTheThing method, and false from the public isValid method.

Edit: And then to implement isValidInternal, you have the logic for testing validity once, but either return false or throw an exception as per the throwIfInvalid flag.

Answer 3

What if your create a isValidParam method that returns a bean like:

class ValidBean {
  private boolean isValid;
  private Exception exceptionOnInvalid;
  public ValidBean(boolean isValid, Exception exceptionOnInvalid) {...}
  // accessors
}

With the method being:

private ValidBean isValidParam(SomeParameter sp) {
       if (sp == null) {
          return new ValidBean(false, new ParameterCannotBeNullException());
       }
       if (sp.getNonZeroInt() == 0) { 
         return new ValidBean(false, new ShouldBeNonZeroException());
       }
       if (!sp.someOtherCondition()) {
          return new ValidBean(false, new SomeConditionNotMetException());
       }
       …
       return new ValidBean(true, null);
    }

And then you do:

public boolean isValid(SomeParameter sp) {
 return isValidParam(sp).isValid();
}

And

public void doTheThing(SomeParameter sp) throws SpecificRuntimeException {
  ValidBean validbean = isValidParam(sp); 
  if (! validBean.isValid()) {
      throw validBean.getExceptionOnInvalid();
  }
  ...
}

Answer 4

The fact that your code checks the arguments for sanity and returns runtime exceptions is a strong indication that you want to report programming error, i.e. your API is used/called in a way it should not be used/called. Since you could expect that your API is called with valid arguments, I wouldn't expect that your second solution to be problematic.

P.S.: You should not use runtime exception types in a method's throws declaration. But you should state those runtime exceptions in the method's Javadoc @throws documentation.

Answer 5

This can be a murky issue; when it comes to exceptions in APIs I've never found a really clear, logical, satisfactory answer to every case.

Do you expect every user of isValid() to be using it to guard a subsequent call to doTheThing()? If so, the client code would be clearer to use the standard try/catch idiom instead, and you may not need an isValid() method at all.

(You'd still have to ponder whether the exception should be checked or unchecked; the line can be a fine one and I won't go into it here.)

And if you don't see isValid() being used this way, then just implement it with a try/catch of your own; I know it feels dirty, but it's better than the alternatives!