RFR [8014066] Mistake in documentation of ArrayList#removeRange

[Ivan Gerasimov]

Thanks David!

On 14.03.2014 12:31, David Holmes wrote:
>
>>
>>> Second point is that the condition "fromIndex >= size()" is already
>>> captured by the condition "if {@code fromIndex} or {@code toIndex} is
>>> out of range". By definition fromIndex is out-of-range if it is < 0 or
>>> >= size(). So I don't see any error here even if there is some
>>> redundancy.
>>>
>> I don't think that 'fromIndex < size()' restriction is quite natural.
>
> Seems perfectly natural to ensure the fromIndex is somewhere in the 
> available range.
>

Sorry, what I meant was that it is too restrictive.
'fromIndex <= size()' is indeed natural.

>
>>> And finally, the AbstractList.removeRange change:
>>>
>>> +      * @throws NoSuchElementException if {@code (toIndex > size())}
>>>
>>> seems to reflect an implementation accident rather than a clear API
>>> design choice. If the toIndex is out of range then
>>> IndexOutOfBoundsException is what should be thrown. Otherwise you
>>> constrain any subtypes that override this to throw an exception type
>>> that only has meaning with the AbstractList implementation strategy -
>>> and by doing so you are making ArrayList.removeRange violate this new
>>> specification.
>>>
>>> It is unfortunate that the existing specification(s) for removeRange
>>> are lacking this key detail on exception processing, and lack
>>> consistency between AbstractList and it's sublcass ArrayList. I think
>>> ArrayList has the better/cleaner specification and that should be
>>> pushed up AbstractList and AbstractList's implementation should be
>>> modified to explicitly check the pre-conditions.
>>>
>> Again, I agree with you.
>> One thing I noticed is that some methods I mentioned above
>> (List.subList(), Arrays.sort(), etc) throw IllegalArgumentException when
>> fromIndex > toIndex, not IndexOutOfBoundException.
>> Wouldn't it be more correct to adopt this into removeRange() too?
>
> Unfortunately there are a lot of inconsistencies in the API design 
> throughout the JDK - even in classes that are somewhat related. That 
> said List.subList throws IOOBE, But I do see examples of:
>
>     IllegalArgumentException - if fromIndex > toIndex
>
> which is definitely more correct if both values are in range, but then 
> what if both are out of range?
>
> Anyway you can't just arbitrarily make changes to the specifications 
> of these methods - even if trying to fix obvious inconsistencies. The 
> main priority is that specs and implementations agree; and that specs 
> in subtypes are consistent with the spec in the supertypes.
>
> I see a number of issues here that may be tricky to reconcile having 
> regard for compatability.
>

What we have how is:

AbstractList#removeRange()
- spec misses throws section;
- implementation has a bug: the function may throw 
NoSuchElementException instead of IndexOutOfBoundsException;

ArrayList#removeRange()
- spec contains wrong condition 'fromIndex >= size()' ;
- implementation has a bug: the condition fromIndex <= toIndex isn't 
checked at all.

Thus, we'll need to adjust both spec and implementation of both base and 
derived classes.
That's why I ask, if it may make sense to add IllegalArgumentException 
for fromIndex > toIndex case, since the spec needs to be corrected anyway.
Of course, no problem to leave only IndexOutOfBoundsException.

Concerning the ambiguity, which exception to throw if toIndex < 
fromIndex and some of them is out of range.
I believe, the conditions should be checked in the order they're listed 
in the spec.
Therefore, IndexOutOfBoundsException in this case.

Sincerely yours,
Ivan