commons-lang
commons-lang copied to clipboard
apache
Some ArrayUtils method descriptions are not accurate
ArrayUtils.subarray(final boolean[] array, int startIndexInclusive, int endIndexExclusive)
The change applied to the subArray(boolean[], ...) also applies to 7 other subArray methods in the same class. Can you update to fix them all. Thanks.
Note that the javadoc mixes the use of what happens when a parameter is 'undervalue' (too small) and 'overvalue' (too large), with the effect on the parameter (undervalue set to 0 for start index; overvalue set to array.length for end index), or the effect on the result (overvalue produces an empty array for start index; undervalue produces an empty array for end index). It is a bit strange.
However for the description of what produces an empty array then if we change to Undervalue (<= startIndex) produces empty array we should also change to overvalue (>= array.length) results in an empty array. The two should also consolidate on either results in an empty array or produces (an) empty array.
Ok，I'll try it tomorrow.
---- Replied Message ---- | From | Alex @.> | | Date | 06/13/2023 19:24 | | To | @.> | | Cc | @.>@.> | | Subject | Re: [apache/commons-lang] fix: Method description is not accurate (PR #1065) |
The change applied to the subArray(boolean[], ...) also applies to 7 other subArray methods in the same class. Can you update to fix them all. Thanks.
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>
I think it is fine to just change method description to make it precise. It's not necessary to change method itself. and what do you say?
No need to change the method. But I suggested changing the description of the startIndexInclusive too since the empty array result is for >= not >:
/**
* ...
* @param startIndexInclusive the starting index. Undervalue (<0)
* is promoted to 0, overvalue (>= array.length) results
* in an empty array.
* @param endIndexExclusive elements up to endIndex-1 are present in the
* returned subarray. Undervalue (<= startIndex) results in an
* empty array, overvalue (> array.length) is demoted to
* array length.
* ...
*/
@RestartCoding This does not seem complete in the sense that the "overvalue" comments @aherbert mentions are not addressed.
@aherbert Thoughts on closing this PR or merging it despite the shortcomings?
I would close it. The javadoc currently reflects what the code is doing. If you read the javadoc as is you should get the understanding that the method is trying to protect you from a mess up for the range of the index arguments. However it does not stop real stupidity. You can do this and create a huge array via negative overflow:
int[] a = {0};
// Tries to create an array of size Integer.MAX_VALUE !
subarray(a, 1, Integer.MIN_VALUE);
A caller may soon find out this is a bad call due to either memory allocation error, or an error from the subsequent arraycopy which could fail as the source array is too small. If the source array happens to be huge as well then you could end up copying something. But currently this method does not protect from such bad arguments.
This method is from lang 2.1 (circa Nov 2005). Arrays.copyOfRange is in JDK 1.6 (circa Dec 2006) and does the copy correctly if you specify valid indices. Since this is doing checks to make it null safe and index safe we wish to avoid delegating to copyOfRange after we have clipped indices to avoid duplicating index checks; we also wish to return a singleton empty array if possible. A fix would be to remove the check on the size and do:
if (endIndexExclusive <= startIndexInclusive) {
return // empty array
}
// Now compute size...
I think this would maintain the intention of the method. To allow you to call with basically any argument and get either a null array, empty array or a populated (sub-)array if you happen to have covered some/all of the range of the input array with the arguments. If you remove this intention, then you may as well just deprecate in favour of copyOfRange.