views:

92

answers:

2

Today I encountered the following situation: ("pseudo code")

class MyClass {

    public void workOnArray(Object[] data) {
        for (Object item : data) {
            workOnItem(item);
        }
    }

    public void workOnItem(Object item) {
        if (item == null) throw new NullPointerException();
    }
}

Now if the caller calls workOnArray with an array containing null items, the caller would get a NullPointerException because of workOnItem.

But I could insert an additional check in workOnArray, in other words, the problem can be detected sooner.

(Please note that this is a trivial example, in a real life application this can be much less obvious).

On the pro side I'd say an additional check could offer more high-level diagnostic information. Also failing early is always a good thing.

On the contra side I'd ask myself "If I do that, shouldn't I also validate each and every parameter passed into the core API of my programming language and throw the exact same exceptions?"

Is there some rule of thumb when to throw exceptions early and when to just let the callee throw it?

+8  A: 

In the case of a loop processing items like that, there's one thing that would definitely make me want to pre-validate the whole array of items first; If it would be bad for some of the items to be processed before an exception was thrown, leaving any remaining items un-processed.

Barring some sort of transaction mechanism wrapping the code, I would usually want to have some assurance that the items in the collection were valid before beginning to process them.

Andrew Barber
+1. If you are changing data and don't have transactions available, check first (If it is just a calculation with no side effects, don't worry too much). In the loop situation, I am also always thinking (and never executing) about not just throwing an Exception that says "this element is bad", but collecting all the errors in an Exception that says "elements one, five, seven are bad".
Thilo
@Thilo +1 to that, too; it adds a bit of complexity, but it may be an excellent option to do something like continuing to process the rest of the elements, and then return a collection of exceptions (perhaps in a custom exception with a collection member) or an exception that includes all the error info and allows only the 'bad' items to be fixed and tried again.
Andrew Barber
+1 I agree that pre-validation is a plus, even if it involves checking of complex structures.@DR: Try to think about this problem in terms mock tasting: If I would like to mock `workOnItem()` and check the functionality for `workOnArray()` only, then the behavior will depend on what position contains `null` element. Complicated. If validation is separated from business logic, the testing is simpler.
dma_k
+2  A: 

In this example, the workOnItem method is the one that cares whether or not item is null. The workOnArray method doesn't care whether or not items are null and so IMO shouldn't validate whether or not any items are null. The workOnItem method does care and so should perform the check.

I would also consider throwing a more appropriate exception type from workOnItem. A NullPointerException (or in C#, NullReferenceException) often indicates some unexpected flaw in the operation of a method. In C#, I would be more inclined to throw an ArgumentNullException that includes the name of the null parameter. This more clearly indicates that workOnItem can't continue because it cannot handle receiving a null argument.

John Bledsoe
"The workOnArray method doesn't care whether or not items are null ". Well, if the third item is null, the first two will already have been processed, which might leave if bit of an inconsistency behind if you do not have transactions. On the other hand, that could happen no matter how much error checking you do in advance.
Thilo
@Thilo - Certainly true. This is where abstract solutions break down and real business requirements start to dictate what is necessary. Certain applications may require that all items in a batch be processed or none at all, while other applications may benefit from partial success. The ultimate answer depends on the business rules of the application.
John Bledsoe