Error checking in business methods a.k.a doing defensive programming

Question

I am starting to add tests to a large java code base. I often see the following in the session beans I am testing:

public OrderDTO getOrderDTO(Long id) {
    Order o = (Order)entityManager.find(Order.class, id);
    OrderDTO dto = new OrderDTO(o.getId(), o.getCurrency());
    return dto;
}

Its quit easy to write a unit test to break this code (send in a null or a non existing id). When I did that half the developers of the team said:

We are not error checking everything. If you parameter is rubbish you will know fast!

The other half said:

We must add ifs to the id and then to the o and if any of them are null the null is what we return.

Isn't the point of unit testing to find exactly thees kind of issues? (Yes, I am asking for an opinion!)

Yes, switching from Long to long will remove one if.

score 2 · Accepted Answer · answered Aug 17 '15 at 12:12

2

While this is somewhat opinion based, few people would say it's correct to return null if given null as a parameter. If I were to add anything, it would be at most a IllegalArgumentException (or even NPE) when null is passed in.

A test could be created to check that the method fails in a consistent fashion, but it would really be testing the behaviour of the JPA provider and not your code.

answered Aug 17 '15 at 12:12

Kayaman

72,141
5
83
121

So you would add an *Exception and keep the code as is? My take on it is to change Long to long in the method parameter and keep the code as is. – user1329339 Aug 17 '15 at 12:26
@user1329339 changing the type from Long to long is really not that significant, null just resolves to zero. – NimChimpsky Aug 17 '15 at 12:27
1

@user1329339 I wouldn't add anything. I would make integration tests that verify that it's not possible to do something that would result in a `null` being passed to this method. – Kayaman Aug 17 '15 at 12:34
@Kayaman Thanks for your response. I have added tests checking that errors are thrown when passing null and I will add the test you suggest. – user1329339 Aug 17 '15 at 13:00
@NimChimpsky Well, I do think it does improve a bit since a primitive indicates (to the dev writing the code to use this method) that null is not ok to use as a parameter. But as you say, not very significant. – user1329339 Aug 17 '15 at 13:03
@user1329339 yes true, regarding primitives, however it would also fundamentally alter the behavior of the code. – NimChimpsky Aug 17 '15 at 13:11
@NimChimpsky Sorry but I am not following you here. What would alter? – user1329339 Aug 17 '15 at 13:41
1

@user1329339 a null value would return the record associated with id 0 - which may work for you, but also may not. – NimChimpsky Aug 17 '15 at 14:48
@NimChimpsky Ok, then I understand what you mean. In our case it would not change the meaning. Thanks for responding. – user1329339 Aug 18 '15 at 07:36

score 1 · Answer 2 · answered Aug 17 '15 at 12:26

1

Returning nulls should be avoided, they are the source of all evil.

You could use the Null Object Pattern.

Or throw an exception, illegalargument or entitynotexsits spring to mind.

If you must return null, at least wrap it in an optional or use guava.

answered Aug 17 '15 at 12:26

NimChimpsky

46,453
60
198
311

score 0 · Answer 3 · answered Aug 17 '15 at 13:45

As always, it depends :)

If you are writing library code (code that is shared and used elsewhere or even by others), then you should really aim for handling all thinkable input values in a consistent way. Using well documented exceptions instead of returning null is certainly preferable for library code.

On the other hand there is local code. I can see the method is public, but that does not eliminate the possibility that it's only used in one isolated portion of the code base.
In that case, you don't work with assumptions about parameters and what the caller expects in return. You work with defined calls. You control what is sent in and how the caller handles the return value. So it's OK not to null-check, if you know the caller never sends null. And it can be OK to return null as well if that simplifies your overall program structure.

ps: what bothers me most in that method is the unhandled NPE if entityManager.find(..) fails :)

Error checking in business methods a.k.a doing defensive programming

3 Answers3