On 3 October 2013 23:08, Xueming Shen <xueming.s...@oracle.com> wrote:
> (2) It appears we started to use "endExclusive" in "until() methods, while
> the param
> has been renamed to be "exclusive" explicitly, personally I prefer
> still keep the
> word "exclusive" in its spec, or at least we want to be consistent, for
> example
> LocalDate.until(ChronoLocalDate) still keeps the "exclusive" after
> renaming.
The "exclusive" in the parameter name is for IDE autocompletion. I'm
surprised I didn't put it in before as its my long standing practice
and very useful for users. The Javadoc should also state exclusive.
> On 10/03/2013 02:03 PM, roger riggs wrote:
>> On 10/3/2013 2:38 PM, Xueming Shen wrote:
>>> (1) until(Temporal endExclusive, TEmporalUnit unit)
>>> -> Shouldn't we invoke requireNonNull() for unit before invoking
>>> unit.between(...)?
>>
>> There is no doubt that invoking unit.between() will cause an NPE if unit
>> is null.
>> Adding requireNonNull will only improve the error message and add bulk to
>> the code.
>
> Most of the use cases of requireNonNull here are "no doubt that a NPE will
> be triggered"
> and the only benefit of having them is to improve the err msg and there for
> add bulk to
> the code. What I'm saying here is the consistency.
There are lots of places in 310 where we have Objects.requireNotNull
and lots of places where we don't. Where we don't the minimum should
be an early throw of NPE that would be simple for a user to identify.
If it would be complex to identify then requireNotNull is useful.
So, not entirely consistent, but not completely inconsistent either...
>>> (3) We have the requireNonNull check pattern below in most places
>>> if (X instanceof Y) { ... }
>>> Objects.requireNonNull(X, "x");
>>
>> Isn't that just useless, instanceof only returns true for non-null
>> argument.
Yes, that check is useless.
> I may not have stated my observation clearly enough. In most of the cases
> the code inside {...} returned. My guess of intent of the original code is
> to
> optimize the case of non-null parameter AND the X is the instance of Y,
> which
> should be normal use scenario. But the plus/minus implementation appears
> not try to optimize this (null always triggers false for instanceof) for
> case that
> the amountToAdd is indeed a Period. This is also related my argument in (1),
> why we check requireNonNull here but not in (1). Well, not a big deal
> though.
In general, if there is a sensible order of code for minor
optimisation I tried to do it. But it might not be consistent.
Stephen
