Should we allow null/empty parameters?

Question

I recently had a discussion with a co-worker on whether we should allow null or empty collections to be passed as method parameters. My feeling is that this should cause an exception, as it breaks the method's 'contract', even if it does not necessarily break the execution of the method. This also has the advantage of 'failing fast'. My co-worker argues that this leads to littering the code with 'not null/not empty' checks, even when it wouldn't really matter.

I can see his point, but allowing null or empty parameters makes me feel uneasy. It could hide the true cause of a problem by delaying the failure!

Let's take two concrete examples:

1) Given we have an Interval class with an overlaps(Interval) method, what should happen if a null is passed as the parameter? My feeling is that we should throw an IllegalArgumentException to let the caller know something is probably wrong, but my co-worker feels returning false is sufficient, as in the scenarios where he uses it, it simply doesn't matter if the second Interval is null or not (all that matters is whether they overlap).

2) Given a method like fetchByIds(Collection ids), what should happen if an empty collection is provided? Once again I'd like to warn the caller that something abnormal is happening, but my coworker is fine with just receiving an empty list, as once again he doesn't really care whether there are any ids or not.

Where does the responsibility of the called code end? In both cases the calling code didn't mind whether the parameter was null or empty, but in other scenarios this might point to a likely bug. Should a method only guarantee that it won't break as long as the preconditions are adhered to, or should it try to identify potential buggy invocations as well?

EDIT: I see a lot of good answers and most tend to say define it as a contract/in the documentation and stick with it, but I'd like your opinion on when to allow it and when not (if ever). In the specific examples, what would you do? Given that for 90% of the uses, not validating the input will be fine, will you still validate to flush out bugs in the remaining 10%, or will you rather address those as they appear and avoid the unnecessary null/empty checks?

Answer 1

I guess there is no right or wrong to this, but when you will throw an exception this means that something unforeseen has occurred from which you can't recover. For example a method should write something to a disk and that disk is full... So you should ask yourself: is there a way around this problem without throwing an exception. Another question is also is this functionality included in this class?

For example if you have a method called PrintList() and you have an argument that could be null, is it the responsibility of the method to get the list, and then print it or should it only print it? Maybe the name of the method should be changed to include this responsibilty: GetAndPrintList().

Also ask yourself what would happen if a new member is added to the team: will he find the code easy to understand and read without extra documentation?

Answer 2

It is indeed a problem of context and there is no good answer. I'd go with the principle of "least amount of surprise" to choose. Consider the people using the code:

• if the code is to be used internally, consider the team that will be using it: do they know how to handle the potential exceptions? do the framework allow easy exception-processing? does the team use it? If you're the only one worrying about this kind of problems, perhaps it's better to roll with it and handle empty parameters/lists

• if you're building an API, you're pretty much in charge, but you have to be consistent. If some methods allow null parameters and others don't, it may be the sign of a bad design.

• if you're building some code to interact with the UI, make sure the UI has ways of chowing the error down to something understandable, and make sure the UI designer know what to expect. If they don't disable the "Send To Users" button when the user list is empty, how should they handle it?

So, sorry, i don't think there's a Right Way here. As for me, i'd go with the throw exception school of thought whenever possible.

Answer 3

My general approach is to not accept null. An empty collection is fine.

These are general rules, and there are cases where it makes sense to break those rules. In those cases, that is what method documentation is for. The method defines the contract, the documentation is the fine-print.

Answer 4

Your cases are two different situations that map nicely to the real world:

1) Given we have an Interval class with an overlaps(Interval) method, what should happen if a null is passed as the parameter? My feeling is that we should throw an IllegalArgumentException to let the caller know something is probably wrong, but my co-worker feels returning false is sufficient...

Passing a null here is like asking "What's the difference between a duck?", which you can't answer because there's information missing. You can't punt and say "there's no difference" because you have no idea whether the missing information is another duck (no difference) or a water buffalo (big difference). If the contract stipulates that the caller has to provide something to compare and the caller didn't hold up its end of the bargain, that's a good a reason as any to throw an exception.

2) Given a method like fetchByIds(Collection ids), what should happen if an empty collection is provided?

This is akin to your wife telling to you grab the shopping list off the refrigerator and pick up everything on it. If there's nothing on the list (empty collection), you come home with nothing and you've done exactly what you were asked. If you go to the refrigerator and don't find the shopping list (null), you raise an exception by telling your wife there's no shopping list there and she can decide if she really meant to tell you it was on the kitchen table or to forget the whole thing.

Should a method only guarantee that it won't break as long as the preconditions are adhered to, or should it try to identify potential buggy invocations as well?

As others have said, the method should guarantee that it will behave however its documentation says it will behave. If the contract says the method will return a certain result in the presence of a null argument, it's up to the caller to make sure it knows it's passing a null and be able to deal with the results.

How a program should behave in the presence of plausible-but-fishy data depends on a lot of things, like how important it is for it to continue functioning or whether continuing to process that kind of data will have an adverse impact on anything else. That's a decision for you, the developer, to make on a case-by-case basis using your judgment. Anyone who purports to know that it should always be one way in every circumstance hasn't examined the problem closely enough.

In cases where you elect to not throw an exception (e.g., returning false for overlaps(null)), you always have the option of logging the fact that you saw something questionable along with whatever other information you have available (stack trace, etc.). This gives you the option of dealing with it out-of-band so the program will continue functioning.