On Dec 13 2012, at 22:28 , David Holmes wrote:
>> I have added @throws NPE for a number of the default methods. We won't be
>> including @throws NPE in all cases where null is disallowed because when the
>> @throws NPE is declared the API is required to throw NPE in that
>> circumstance. So for cases where the NPE is "naturally" thrown or that
>> aren't performance sensitive we will likely add @throws NPE declarations but
>> for performance sensitive methods we won't be adding explicit null checks to
>> match a @throws NPE specification. There's a tradeoff here in some cases.
>> Please feel free to quibble about specific cases as they occur. :-)
>
> That doesn't make sense to me. The throwing of the NPE is intended to be part
> of the specification not an implementation choice. Further @param foo
> non-null, is just as binding on implementations as @throws NPE if foo is
> null. ???
An "@param foo non-null" by itself is not considered normative because it
doesn't document what happens when null is passed. So it ends up being advisory
only. An "@throws NPE" is considered normative and the implementation must
throw in all circumstances described.
(Please bear with the step-by-step nature of the following sample, it's
incremental pace is not meant to be insulting--it's a write-up for a general
FAQ). If I have a method:
/**
* If the object is visible then write it's string form to the provided
PrintStream.
*
* @param bar destination for display
/
public void display(PrintStream bar) {
if(visible) {
bar.write(toString());
}
}
This implementation isn't going to work well if "bar" is ever null. So I
document that in the "@param bar":
/**
* If the object is visible then write it's string form to the provided
PrintStream.
*
* @param bar destination for display, must be non-null
/
public void display(PrintStream bar) {
if(visible) {
bar.write(toString());
}
}
The "@param bar" documentation now says that it must be non-null but doesn't
explain what happens if null is passed. Having documented that null shouldn't
be passed is helpful but not as helpful as it could be. To specify what happens
I must add "@throws NPE":
/**
* If the object is visible then write it's string form to the provided
PrintStream.
*
* @param bar destination for display, must be non-null
* @throws NullPointerException if bar is null
/
public void display(PrintStream bar) {
if(visible) {
bar.write(toString());
}
}
This implementation would superficially seem to conform to the contract
described in the javadoc. However, when the "if(visible)" conditional is false
then no NPE will be thrown. Contract broken. It is required to add an explicit
null check on "bar" ie.
/**
* If the object is visible then write it's string form to the provided
PrintStream.
*
* @param bar destination for display, must be non-null
* @throws NullPointerException if bar is null
/
public void display(PrintStream bar) {
Objects.requireNonNull(bar);
if(visible) {
bar.write(toString());
}
}
Adding the "Objects.requireNonNull" ensures that the NPE is thrown in all
appropriate cases. There is also another alternative:
/**
* If the object is visible then write it's string form to the provided
PrintStream.
*
* @param bar destination for display, must be non-null
* @throws NullPointerException if bar is null and the component is visible.
/
public void display(PrintStream bar) {
if(visible) {
bar.write(toString());
}
}
The conditions in which NPE are thrown are amended so that throwing is only
required if the component is visible. Documenting the conditions could quickly
become complicated if display was a more involved method. At some point it
becomes easier to just add an explicit check. It can also be useful to add
explicit NPE checks as pre-conditions before a multi-stage operation where a
late stage NPE might corrupt the object state.
In a very few cases an explicit null check might add too much overhead to a
performance sensitive method and writing an accurate "@throws NPE" isn't
possible (perhaps because of delegation) then the "@throws NPE" should be
removed and only the advisory "@param bar ... must be non-null" will have to
suffice.
Mike
> I think defining the NPE via the @param and @throws is a lose-lose situation:
>
> ! * @param left {@inheritDoc}, must be non-null
> ! * @param right {@inheritDoc}, must be non-null
> ! * @return {@inheritDoc}, always non-null
> ! * @throws NullPointerException if {@code left} or {@code right} is null
>
> You only need one convention.
>
> David
> -----
>
>
>> Mike
