Should exceptions be checked at lower level if higher level takes care of them

Question

This is a quote from Effective java.

"Where possible, the best way to deal with exceptions from lower layers is to avoid them, by ensuring that lower-level methods succeed. Sometimes you can do this by checking the validity of the higher-level method’s parameters before passing them on to lower layers."

Consider an object called "AccessControlContext actx" which is propogated from handler to lower levels. We can do a higher level check that "actx != null" but does it need to done in lower level again ?

Eg in psuedocode:

class RestServlet {
   void verify Servlet(AccessControlContext actx) {
        // check made at higher level according to effective java
        if (actx == null) { throw exception; }  
        // do something
        Verify.checkAccessControl(actx); // calling lower leve
        // do something
    }
}

class Verify {
   static checkAccessControl(actx) {
        // DO I NEED NULL POINTER CHECK HERE AGAIN ?
   }
} 

Although question is nested inside the comment, reiterating with elaboration. A redundant check at lower level ensures defensive coding but - it is redundant. It may be well spelled out in the javadocs that it does not accept null, but that does not solve the purpose of having a bug free code. Should we dedupe exception checks ?

NOTE: This was a random example. My question is not specific to this example. Its a broader question which seeks to understand how much to duplicate exceptions.

Answer 1

When it comes to validating parameters, in my opinion you can't do it too often. The way I'm used to working, parameters are validated in all layers of the code. If a method can't handle a null value (it would lead to a nullpointer for instance), it should be up to the method itself to make sure that the parameters passed aren't null. The same goes for methods where a bad parameter value would cause and exception further down in the layers.

The way I see it, if a bad parameter would cause an exception, it doesn't matter whether or not it is an exception thrown from a parameter validation at a high level or a parameter validation at low level, it is anyways better than the exception it would cause if there were no validation at all (or even worse, no exception and an erroneous execution.)

By letting each method be responsible for validating its input, even if it is just a passthrough-method, one can make sure that any new method calling an existing method will be prevented from passing along a bad parameter, since it would lead to a validation exception, no matter if the method being called is a high level or low level method.

So, in your example I would validate the input in both methods, with accompanying tests verifying that the validation behaves as it should. Input validation could also be as easy as a one-liner, check out the Validate class of apache commons lang: http://commons.apache.org/proper/commons-lang/javadocs/api-2.6/org/apache/commons/lang/Validate.html

For instance:

import org.apache.commons.lang.Validate;
import org.apache.commons.lang.StringUtils;

public class Something {
   public void doSomething(final Integer someInt, final String someString) {
       Validate.notNull(someInt, "Can't proceed when someInt is null!");
       Validate.isTrue(!StringUtils.isEmpty(someString, "Can't proceed when someString is null or empty!");
       //do stuff
   }
}

If the validation fails, an IllegalArgumentException with the message defined as the second parameter will be thrown. Tests could then look like this:

public class SomethingTest {

    private Something something;

    @Rule
    public ExpectedException expectedException = ExpectedException.none();

    @Before
    public void setup() {        
        something = new Something();
    }

    @Test
    public void doSomethingShouldFailIfSomeIntIsNull() {
        expectedException.expect(IllegalArgumentException.class);
        expectedException.expectMessage("someInt is null");

        something.doSomething(null, "hello");
   }

   @Test
   public void doSomethingShouldFailIfSomeStringIsEmpty() {
        expectedException.expect(IllegalArgumentException.class);
        expectedException.expectMessage("someString is null or empty");

        something.doSomething(123, "");
   }

   //more tests..
}

Answer 2

I'm not sure that I'd be happy with this approach, what if checkAccessControl is called from many places? You have to rely on each caller doing the null check - it's not a safe way to proceed. I'd be putting the null check in to the checkAccessControl method, probably throwing IllegalArgumentException if a null was detected.

So, my broad approach would be, if your method relies on specific values being presented to it ( eg a parameter can't be null ), I would make it the responsibility of the method to validate it.

You can of course still ask the caller to validate the argument for you ( via javadocs for example ), but you can't enforce it.