RFR : CR8004015 : Add parent interfaces and default methods to basic functional interfaces

[David Holmes]

On 15/12/2012 4:58 AM, Mike Duigou wrote:
>
> 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.

Aside: that is an interesting interpretation but from whence does it 
come? It is non-normative by definition because it is incomplete? Or is 
it just non-normative because it is an @param tag?

> (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:

But once you add the @throws the advisory of the @param is redundant. 
Hence to me it is an either/or situation. Further the advisory, being 
advisory, is useless in my opinion, so not something to use regardless.

David
-----

> /**
> *  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
>