Java: Why not allow nulls in methods to represent optional parameters

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

Technically, what you should test depends on your test goals. But in general, you should try to test everything that can go wrong. Here:

this.coll = coll == null ? null : coll.clone();

• You might have forgotten to use the coll parameter, i.e. the above statement is missing. This can be checked by a test that depends on the coll provided to the constructor.

• You might have forgotten to clone the parameter:

  this.coll = coll;
  

  This can be checked by a test that modifies the parameter coll and compares the result with the object-owned coll. They should be different after the modification.

• You might have forgotten to handle the null case:

  this.coll = coll.clone();
  

  This can be checked by a test that omits this parameter/provides a null value. Usually we'd expect an exception, here a null is OK.

• Another thing that could go wrong is that you create a null object or default instance when a null is encountered, e.g.:

  this.coll = coll == null ? new Collection() : coll.clone();
  

  If your code is correct, you want to ensure that an actual null here, not a default instance.

That's already four test cases that would be sensible for this simple line of code. Using a code coverage tool can help to detect uncovered cases in your code, in particular if you also look at branch coverage. Some tools have problems with expression-level control-flow (?:, &&, ||) so it's better (and more readable for humans, too!) to use statement-level conditionals:

if (coll != null) {
    coll = coll.clone();
}
this.coll = coll;