1

Lets say we have a service method getById(Long id) which returns an entity based on its id. What would be the proper action to do in case id is null?

Throw IllegalArgumentException?

Throw NullPointerException? (guavas Preconditions.checkNotNull does this)

return null?

Since there can never be an entity with id == null, returning null does not seem that bad? I mean if the id does not exist the method will return null anyway.

Preconditions are nice one-liners but throwing a NullPointerException seems extreme in this case.

Whats the "best practice" here?

1
  • 1
    Why can't the method signature be getById(long id) instead to push the NPE to the service consumer's code? Mar 16, 2013 at 9:04

3 Answers 3

3

Passing null to such a method indicates a bug. Nobody would ever want to find the entity which has a null ID, since no such thing can exist. So this probably mean that there has been a binding problem in the UI layer, or that the caller forgot to add a hidden ID field in its form, or whatever.

Returning null hides the bug, or makes it more obscure. Throwing an exception spots the bug early, with a clear error message, which allows fixing it early, and makes the application ultimetely more robust.

The convention for non-nullable arguments which are null is to throw a NullPointerException. That's what I'd do.

1
  • I chose this answer. Makes the most sense as likely passing in null is a hint of a problem somewhere else.
    – beginner_
    Apr 4, 2013 at 12:17
1

There is no record with id of null therefore the method should do whatever it does if given a "valid" id for which there is no record. That is the principle of least surprise. The consumer will have coded for the case of not found so they should be covered.

There is one thing, what is the id of a new record that hasn't been persisted yet? That may cause you to deviate your behaviour if it happens to be null

3
  • yes, a new record has id null. But as long as it is not persisted, it does not exist anyway and should not be returned?
    – beginner_
    Mar 16, 2013 at 9:10
  • But if a new record gets handed to some other code without being persisted and then that code tries to do something with it's id you'd fail faster by throwing IllegalArgumentExecption("only persisted records can be found by id") so in that case you might deviate... In any case returning null is often a bad smell, throwing a NotFound or returning an Optional is usually better Mar 16, 2013 at 9:15
  • If new records have id of null then null indicates a new record, and your getById method cannot work for records that have not been persisted, so in that case throw an exception because there are records with a null ID but the service layer could not possibly find them. OTOH if creating a new record had the ID generated each time, then there are no (persisted or not persisted) records with a null ID so the only meaning of such would be "not found". Where you have multiple meanings, "not found" is least important Mar 16, 2013 at 9:22
0

Always it is better to use NullPointerException because nothing was found for the value in ID in the Entity Collection.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.