Check parameters annotated with @Nonnull for null?

Question

We've begun using FindBugs and annotating our parameters with @Nonnull appropriately, and it works great to point out bugs early in the cycle. So far we have continued checking these arguments for null using Guava's checkNotNull, but I would prefer to check for null only at the edges--places where the value can come in without having been checked for null, e.g., a SOAP request.

// service layer accessible from outside
public Person createPerson(@CheckForNull String name) {
    return new Person(Preconditions.checkNotNull(name));
}

...

// internal constructor accessed only by the service layer
public Person(@Nonnull String name) {
    this.name = Preconditions.checkNotNull(name);  // remove this check?
}

I understand that @Nonnull does not block null values itself.

However, given that FindBugs will point out anywhere a value is transferred from an unmarked field to one marked @Nonnull, can't we depend on it to catch these cases (which it does) without having to check these values for null everywhere they get passed around in the system? Am I naive to want to trust the tool and avoid these verbose checks?

Bottom line: While it seems safe to remove the second null check below, is it bad practice?

This question is perhaps too similar to Should one check for null if he does not expect null, but I'm asking specifically in relation to the @Nonnull annotation.

Answer 1

If you dont trust FindBug, it may be an option to use an assertion. This way you avoid the problems mentioned by User MSalters but still have a check. Of course, this approach may not be applicable if such a fail-fast approach is not feasible, i.e. you have to catch any exception.

And to further answer the question whether it is bad practice. Well, this is arguable but I would not think so. I consider this FindBug annotation as a light specification following the design by contract principle.

In design by contract you establish a contract between a method and its caller. The contract consists of preconditions that the caller agrees to fulfill when calling the method and a postcondition that in turn is fulfilled by the method after execution if its precondition is fulfilled.

One of the benefits of this development method is that you can avoid a so called defensive programming style (in which you check for all invalid parameter values explicitly etc.). Instead you can rely on the contract, and avoid redundant checks to increase performance and readibility.

Contracts can be used for runtime-assertion checking which follows the above mentioned fail-first principle in which you want your program to fail if a contract is broken, giving you a clue to identify the source of error. (A failed precondition means that the caller did something wrong, a failed postcondition indicates an implementation error of the given method)

Furthermore, contracts can be used for static analysis, which tries to draw conclusions about the source code without actually running it.

The @nonnull annotation can be seen as a precondition that is used for static analysis by the tool FindBugs. If you prefer an approach such as runtime-assertion checking, i.e. the fail first strategy, you should consider using assertion statements as built-in Java. Or maybe to adapt to a more sophisticated specification strategy using a behavioral interface specification language such as the Java Modeling Language (JML).

JML is an extension of Java in which specification is embedded in special Java Comments. An advantage of this approach is that you can use sophisticated specification, even using a development approach in which you only rely on specification rather than implementation details and use different tools that make use of the specification, such as the mentioned runtime-assertion checking tools, automated generation of unit-tests or documentation.

If you share my point of view of @nonnull being a (light) contract, it would be common practice to not add additional checks because the original idea behind this principle is to avoid defensive programming.

Answer 2

Well, consider why you don't write

this.name = Preconditions.checkNotNull(name);  // Might be necessary
that.name = Preconditions.checkNotNull(this.name);  // Who knows?

Programming isn't black magic. Variables don't suddenly become Null. If you know a variable isn't Null, and you know it's not changed, then do not check it.

If you do check it, some programmer in the future (possibly you) will spend a lot of time finding out why that check is there. The check must be there for a reason, after all, so what are you overlooking?