Defensive Programming: Guidelines in Java

Question

I’m from a .NET background and now dabbling in Java.

Currently, I’m having big problems designing an API defensively against faulty input. Let’s say I’ve got the following code (close enough):

public void setTokens(Node node, int newTokens) {
    tokens.put(node, newTokens);
}

However, this code can fail for two reasons:

1. User passes a null node.
2. User passes an invalid node, i.e. one not contained in the graph.

In .NET, I would throw an ArgumentNullException (rather than a NullReferenceException!) or an ArgumentException respectively, passing the name of the offending argument (node) as a string argument.

Java doesn’t seem to have equivalent exceptions. I realize that I could be more specific and just throw whatever exception comes closest to describing the situation, or even writing my own exception class for the specific situation.

Is this the best practice? Or are there general-purpose classes similar to ArgumentException in .NET?

Does it even make sense to check against null in this case? The code will fail anyway and the exception’s stack trace will contain the above method call. Checking against null seems redundant and excessive. Granted, the stack trace will be slightly cleaner (since its target is the above method, rather than an internal check in the HashMap implementation of the JRE). But this must be offset against the cost of an additional if statement, which, furthermore, should never occur anyway – after all, passing null to the above method isn’t an expected situation, it’s a rather stupid bug. Expecting it is downright paranoid – and it will fail with the same exception even if I don’t check for it.

[As has been pointed out in the comments, HashMap.put actually allows null values for the key. So a check against null wouldn’t necessarily be redundant here.]

Answer 1

In Java you would normally throw an IllegalArgumentException

Answer 2

Your approach depends entirely on what contract your function offers to callers - is it a precondition that node is not null?

If it is then you should throw an exception if node is null, since it is a contract violation. If it isnt then your function should silently handle the null Node and respond appropriately.

Answer 3

The standard Java exception is IllegalArgumentException. Some will throw NullPointerException if the argument is null, but for me NPE has that "someone screwed up" connotation, and you don't want clients of your API to think you don't know what you're doing.

For public APIs, check the arguments and fail early and cleanly. The time/cost barely matters.

Answer 4

If you want a guide about how to write good Java code, I can highly recommend the book Effective Java by Joshua Bloch.

Answer 5

like the other : java.lang.IllegalArgumentException. About checking null Node, what about checking bad input at the Node creation ?

Answer 6

It sounds like this might be an appropriate use for an assert:

public void setTokens(Node node, int newTokens) {
    assert node != null;
    tokens.put(node, newTokens);
}

Answer 7

Different groups have different standards.

Firstly, I assume you know the difference between RuntimeExceptions (unchecked) and normal Exceptions (checked), if not then see this question and the answers. If you write your own exception you can force it to be caught, whereas both NullPointerException and IllegalArgumentException are RuntimeExceptions which are frowned on in some circles.

Secondly, as with you, groups I've worked with but don't actively use asserts, but if your team (or consumer of the API) has decided it will use asserts, then assert sounds like precisely the correct mechanism.

If I was you I would use NullPointerException. The reason for this is precedent. Take an example Java API from Sun, for example java.util.TreeSet. This uses NPEs for precisely this sort of situation, and while it does look like your code just used a null, it is entirely appropriate.

As others have said IllegalArgumentException is an option, but I think NullPointerException is more communicative.

If this API is designed to be used by outside companies/teams I would stick with NullPointerException, but make sure it is declared in the javadoc. If it is for internal use then you might decide that adding your own Exception heirarchy is worthwhile, but personally I find that APIs which add huge exception heirarchies, which are only going to be printStackTrace()d or logged are just a waste of effort.

At the end of the day the main thing is that your code communicates clearly. A local exception heirarchy is like local jargon - it adds information for insiders but can baffle outsiders.

As regards checking against null I would argue it does make sense. Firstly, it allows you to add a message about what was null (ie node or tokens) when you construct the exception which would be helpful. Secondly, in future you might use a Map implementation which allows null, and then you would lose the error check. The cost is almost nothing, so unless a profiler says it is an inner loop problem I wouldn't worry about it.

Answer 8

I think a lot depends on the contract of the method and how well the caller is known.

At some point in the process the caller could take action to validate the node before calling your method. If you know the caller and know that these nodes are always validated then i think it is ok to assume you'll get good data. Essentially responsibility is on the caller.

However if you are, for example, providing a third party library that is distributed then you need to validate the node for nulls, etcs...

An illegalArugementException is the java standard but is also a RunTimeException. So if you want to force the caller to handle the exception then you need to provided a check exception, probably a custom one you create.

Answer 9

Personally I'd like NullPointerExceptions to ONLY happen by accident, so something else must be used to indicate that an illegal argument value was passed. IllegalArgumentException is fine for this.

if (arg1 == null) {
 throw new IllegalArgumentException("arg1 == null");
}

This should be sufficient to both those reading the code, but also the poor soul who gets a support call at 3 in the morning.

(and, ALWAYS provide an explanatory text for your exceptions, you will appreciate them some sad day)

Answer 10

I don't have to please anybody, so what I do now as canonical code is

void method(String s) 

if((s != null) && (s instanceof String) && (s.length() > 0x0000))
{

which gets me a lot of sleep.

Others will disagree.