Search code examples
javaintellij-ideafindbugsjsr305

@Nonnull with different IDEs - warn about unnecessary null checks


I'm working in an environment where developers use different IDEs - Eclipse, Netbeans and IntelliJ. I'm using the @Nonnull annotation (javax.annotation.Nonnull) to indicate that a method will never return null:

@Nonnull
public List<Bar> getBars() {
  return bars;  // this.bars is a final, initialized list
}

I'd like other developers to get a warning if they do one of the following:

  1. Change the method to return null without removing the @Nonnull annotation
  2. Unnecessarily check for null for methods that are explicitly defined with @Nonnull: if (foo.getBars() == null) { ... do something ... }

The first scenario is supported e.g. by IntelliJ. The second is not; the clients are not warned that checking for null is unnecessary.

We're in any case planning to move towards returning clones of collections instead of the collections themselves, so is it better to forget @Nonnull and do this instead:

public List<Bar> getBars() {
  return new ArrayList<Bar>(bars);
}

Edit:

To clarify, I'm not considering changing IDE's for this. I'd like to know whether what I described above is supported by the mentioned IDEs - or alternatively, if there is a good reason as to why it is not supported.

I get the point about not relying too much on contracts. However, if I write getBars() with the style in the last paragraph (return a clone of the list), then e.g. IntelliJ flags a warning for any code that does

if (foo.getBars() == null) { ... }

If you choose to follow this warning and remove the null check, you seem to be equally reliant on the getBars() implementation not changing. However, in this case you seem to be depending on implementation details instead of an explicit contract (as is the case with @Nonnull).

Edit #2:

I'm not concerned about execution speed, null checks are indeed very fast. I'm concerned about code readability. Consider the following:

Option A:

if ((foo.getBars() == null || foo.getBars().size() < MAXIMUM_BARS) && 
    (baz.getFoos() == null || baz.getFoos().size() < MAXIMUM_FOOS)) { 
  // do something
}

Option B:

if (foo.getBars().size() < MAXIMUM_BARS && 
    baz.getFoos().size() < MAXIMUM_FOOS) {
  // do something
}

I think Option B is more readable than Option A. Since code is read more often than it is written, I'd like to ensure all code I (and others in our team) write is as readable as possible.


Solution

  • I would recommend the following:

    • Stick with your @Nonnull and @Nullable annotations just like you are doing it already.
    • Define and enforce a default. It doesn't really matter what the default is, but it should be the same across your entire code base.
    • Use FindBugs to check for your cases 1 and 2. FindBugs has plugins for most IDEs, I saw Eclipse, Netbeans, and IntelliJ mentioned, but there are more. (The number 2 case is covered by the RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE rule. I tested it. Just thought I should mention that after reading DavidHarkness' comment elsewhere on this page.)

    This approach is IDE agnostic and works also in an external build environment like Hudson or Jenkins.