Should I check for nulls in setters and similar methods?

Question

I have plenty of setters in my classes, as well as many methods to add an item or a set of items to lists or maps.

Should I check for nulls for each one of them, or should I allow NullPointerException to happen? This is dangerous for maps though, since I could end up with null values and I wouldn't want that.

So basically, if I have this:

public void setMyMap(Map<Key, List<Value>> map) {
    myMap = map;
}

public void addItem(Key k, Value v) {
    if (map.containsKey(k)) 
        map.get(k).add(v);
    else
        map.put(k, new ArrayList<Value>(Arrays.asList(v)));
}

public void addItems(Key k, List<Value> values) {
    for (Value v : values) addItem(k, v);
}

Etc. Should I do this?:

public void setMyMap(Map<Key, List<Value>> map) {
    if (map != null) myMap = map;
}

public void addItem(Key k, Value v) {
    if (k == null || v == null)
        return;

    if (map.containsKey(k)) 
        map.get(k).add(v);
    else
        map.put(k, new ArrayList<Value>(Arrays.asList(v)));
}

public void addItems(Key k, List<Value> values) {
    if (k == null || values == null)
        return;

    for (Value v : values) addItem(k, v);
}

Side question: I am no fan of using return; just as I did. Normally I would wrap the whole block in an if statement checking if both are not null, rather that exiting the method earlier. Are both practices accepted? Which one should I rather do? Or it's just a matter of taste?

Answer 1

Q: I am no fan of using return; just as I did. Normally I would wrap the whole block in an if statement checking if both are not null, rather than exiting the method earlier. Are both practices accepted? Which one should I rather do?

Exiting the method as soon as possible is ok and much more readable than forcing the developer to read an track the rest of the code and guess what is going to happen at the end of the block. However, in this specific case won't make the code simpler and readable.

Whether you should check nulls or not is up to you. You have to decide whether null is a valid state|value. You will see that it depends on the context. Some times they make sense, others don't. So, you have to think about this carefully every time you face the same dilemma.

If they are valid, it's good to think in a proper representation for them to avoid NullPointerExceptions. The Special Case Pattern suggested by Martin Fowler might help you out here.

If they are not valid values, then keep it simple and throw an exception. A non-checked exception is possible, as for example IllegalArgumentException. Or a checked one if it's treatable and you want to enforce its treatment. The premise is the same for both: fail fast and don't let nulls spread up and down all over the code.

The alternative of waiting for something to cause the NullPointerException is dangerous, because that "something" might not happen. Even if it happens, NullPointerException aren't too dev-friendly. They provide us with very little information. Moreover, the exception might happen much later than those nulls were set. By the time this happens, what caused the error is not as obvious at first glance as it could be throwing an exception with a good message.

Q:Should I do this?

if (k == null || v == null)
   return;

Consider also making the code easier to read too. Do move the condition checking to a method for reuse and expressiveness.

public void addItem(Key k, Value v) {
   checkKeyValuePair(k,y);
   ...
}

public void addItem(Key k, Value v) {
   checkKeyValuePair(k,y);
   ...
}

private void checkKeyValuePair(Object key, Object value){
  if(Objects.isNull(key) || Objects.isNull(value)){
     throw new IllegalArgumentException("Key and value are mandatory");
  }
}

Answer 2

No, you should definitely not do that. You need to make a decision about where null is acceptable. If it's acceptable for the map to be null, then set it to the argument without checking. If not, throw an exception because the caller fucked up and they need to find that out as soon as possible.

Certainly do not set it to some completely random value, like the previous value, a new arbitrary map, or set it to null anyway.

OnErrorGotoNext is a terrible strategy. Don't employ it. If your caller failed to obey the method contract, throw it in their faces.

Answer 3

It comes down to what being passed a null at this point means and what problems it will cause later.

If this method's specification makes it clear that being passed a null is not acceptable then you can justifiably throw a NullPointerException at this time. In this case either change your code above to throw the exception rather than return null or go with your first version and use the Lombok @NonNull annotation.

If it is acceptable that the caller passes null and the method's specification states that it will simply not add the null items, then your second option is fine. However I would possibly recommend putting in a suitable log line in so that you can find out where they are coming from just in case it is unexpected.

The final option is that it is fine to be called with null and that the method specification says will add it to the map. In that case you may still want to put a test in with a log line again simply so that if the inserted null goes on to cause issues in other sections of the code using your collection you can get a trace of what code put it there in the first place.

In short - defensive programming practice makes it sensible to deal with nulls as close to source as possible. Whereas it used to be tedious to plaster code like you listed in all the relevant locations, the Lombok annotation now makes writing defensive code very cheap - certainly far cheaper than the cost of trying to debug why the consumer of your list will occasionally fall over with a NullPointerException.

(Final observation... if the code above is really what you're writing, you should possibly look at the Guava ListMultiMap)

Answer 4

You decide: Is setting the property to nil allowed and how is the behaviour defined? Or is it disallowed and should throw an exception? Or is it a programming error that should immediately assert?

If it’s normal then handle it reasonably. Ignoring it is not reasonable in my opinion. I’d expect the setter to return what the last setter set (usually). Asserting is usually the best, at least during development. If you throw an exception you need to test that thus leads to reasonable behaviour.

(Setting to nil is a pattern used for caches. If nil is stored, the getter calculates, stores and returns a cached property value. If one of the properties the cached value relies on is modified, you set the cached property to nil. )

But if you know that storing nil will cause problems later, you should assert right in the setter so you catch the reason for the problem.