Java – Should Nulls Be Checked in Setters and Similar Methods?

designjavanull

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?

Best Answer

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");
  }
}
Related Topic