Code review: 7012540 (java.util.Objects.nonNull() incorrectly named)
scolebourne at joda.org
Thu Jan 27 08:38:23 UTC 2011
As I said before, removing this method is the best option now. Get it
right in v8.
On 27 January 2011 05:05, <mark.reinhold at oracle.com> wrote:
> Executive summary: requireNonNull is the preferred name.
>> Date: Tue, 25 Jan 2011 18:33:47 -0500
>> From: brian.goetz at oracle.com
>> mark.reinhold at oracle.com wrote:
>>> I think requireNonNull(x) is confusing.
>> Remember there's two versions of someModifierNonNull being discussed; the one
>> currently in Objects is the precondition-enforcing variety, not the
>> postcondition-ensuring variety. Are we talking about the same thing?
> Yes, i.e., the former.
>>> For those familiar with the "requires/ensures/modifies" triad of verbs as
>>> a compact way of identifying the preconditions, postconditions, and side
>>> effects of a method or procedure in a comment, a specification, or a more
>>> formal design-by-contract framework, "requires" is just wrong.
>>> When analyzing the invocation of foo in your example, the non-nullity of
>>> s and t are preconditions of foo and therefore postconditions of the
>>> check method. Naming the check method "requireNonNull" suggests that
>>> the check method itself has a precondition that its argument be non-null,
>>> when in fact it's the check method's postcondition which ensures that
>>> Since postconditions are labeled "ensures" in the "r/e/m" triad, this
>>> method should be named "ensureNonNull".
>> Right, there's precedent for "ensureXxx" methods to actually change the state
>> of things to ensure the postcondition, such as ensureCapacity() methods in the
>> collection implementations. Given that a part of the motivation for this
>> change was to leave room in the namespace for both the precondition-enforcing
>> variety (barf on null) and the postcondition-ensuring variety, aka the
>> carpet-sweeping variety ("if it is null, make it non-null") ensureNonNull
>> sounds a lot more like the the carpet-sweeping version than the version being
>> discussed (barf on null).
> I was actually arguing that the barfing version should be named
> (Wow, am I actually writing about barfing methods?)
>> The r/e/m framework seems to support require for the throwing version (as
>> implemented by this patch): for the throwing version, non-nullity is a
>> precondition of the check method (if the condition is not met, error), whereas
>> for the carpet-sweeping version, it is a postcondition of the check method (if
>> the check method can come up with a reasonable default value). (It happens to
>> be a postcondition of both, but the significant behavior and use of the
>> throwing version currently in Objects is to enforce an error when the
>> precondition is not met.) Therefore:
>> requireNonNull(x) -> throw if x == null
>> ensureNonNull(x) -> convert x to a non-null value if null
>> seems like the right taxonomy.
> It depends on your perspective.
> In a nested method invocation, the postconditions of the inner method
> must relate to the preconditions of the outer method. That is, for the
> to be correct then the postconditions of the bar method must imply the
> preconditions of foo method.
> Now suppose that bar is intended to be the barf-on-null method.
> If we consider how to name bar on its own, outside of its intended usage
> as in this example, then it's natural (for me, anyway) to conclude that
> "ensuresNonNull" is the right name. What's important about bar is its
> postcondition, namely that its return value (x) is not null. Naming it
> "requireNonNull" seems backwards on this view, because "requires" is
> always about preconditions, and it's definitely not a precondition of
> this method that its argument not be null.
> If, by contrast, we consider how to name bar in the context of the above
> example then it's easy to see that "requireNonNull" is the better name,
> because what's important in the overall expression is satisfying foo's
> precondition that its argument not be null, and so
> reads very fluently.
> A little utility method like this will never (well, hardly ever) be used
> in isolation, so I agree now that it makes sense to prefer the second
> form, i.e., requireNonNull.
> I'm not sure I agree that the corresponding carpet-sweeping method is
> best named ensureNonNull; treating that as a conversion, i.e., asNonNull,
> may be a better choice. That, however, is a discussion for another
> - Mark
More information about the core-libs-dev