-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[LANG-1707] Add ArrayUtils.concat methods for concatenating multiple arrays #1519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Hi! This is my first contribution to Apache Commons. |
|
I'm not entirely sure I understand the question correctly. My implementation was based on the Jira ticket and its comments, where adding these methods was discussed and approved (as I understood it). Could you please clarify what I might be missing? I'd be happy to adjust the approach if needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ivamly
I see this could be useful now...
- Add Javadoc tags
@since 3.21.0on new public and protected methods - Remove extra blank lines
- Short-circuit the
totalLengthcomputations to throw anIllegalArgumentExceptiononce the value exceedsSAFE_MAX_ARRAY_LENGTH; you'll likely need to use a long as the running total or useMath.addExact(). - Javadoc: Sentence should end in a period (
@param).
|
Hello @ivamly Are you still working on this? |
|
Hello @garydgregory! I'll push the changes today. |
a33357d to
e11de7c
Compare
|
@garydgregory done. |
e11de7c to
5b8a06b
Compare
|
@garydgregory fix build. |
garydgregory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ivamly
Are you sure about the length computation? It looks to me like it allows for values > SAFE_MAX_ARRAY_LENGTH but just less than Integer.MAX_VALUE.
|
Really, you right. Work on it. |
garydgregory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ivamly
- You are missing tests that check the arithmetic exception path. You can use Mockito or EasyMock to mock
Math.addExact()for that. - The exception
try-catchdoesn't need to be setup and torn down for every array element, instead of you can do that outside theforloop. - You can "move" the array element null test "inside" like this:
totalLength = Math.addExact(totalLength, getLength(array)); - Then you can refactor all of that copy-pasta, like this:
public static byte[] concat(final byte[]... arrays) {
int totalLength = 0;
for (final byte[] array : arrays) {
totalLength = addExact(totalLength, array);
}
final byte[] result = new byte[totalLength];
int currentPos = 0;
for (final byte[] array : arrays) {
if (array != null && array.length > 0) {
System.arraycopy(array, 0, result, currentPos, array.length);
currentPos += array.length;
}
}
return result;
}
private static int addExact(final int totalLength, final Object array) {
try {
return Math.addExact(totalLength, getLength(array));
} catch (final ArithmeticException exception) {
throw new IllegalArgumentException("Total arrays length exceed " + SAFE_MAX_ARRAY_LENGTH);
}
}
...
- Finally, when you add new method, make the new methods appear in AB order, like they existing ones already are.
Thank you!
5b8a06b to
bea4ecd
Compare
|
@garydgregory, thank you for the detailed feedback! I've implemented the changes: added the helper method and sorted the methods. However, I've run into challenges testing the arithmetic exception path. The issue: The project uses EasyMock, which doesn't support mocking static methods. Attempting to test this directly by creating an array near Integer.MAX_VALUE consistently causes an OutOfMemoryError, and such a test likely won't run reliably in CI or for other developers My question: What's the established pattern for testing such scenarios in project? I'd appreciate your guidance on the best path forward. |
|
You can also try Mockito, it's version and BOM POM is inherited from the parent POM. |
|
Mockito can't mock Math.addExact() due to known limitations with |
|
How about creating an addExcat() bridge method then mock that. |
bea4ecd to
885499c
Compare
|
Mockito's mockStatic() proxies the entire class, and we can't call real methods for testing. Created a inner MathBridge class that encapsulates calls to Math and allows mocking only specific operations. |
garydgregory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ivamly
Thank you for your update.
I tested the patch locally and it's green but it's still the case that totalLength computation can be > SAFE_MAX_ARRAY_LENGTH but < Integer.MAX_VALUE.
Implementation of the LANG-1707 suggestion: added the concat methods to ArrayUtils for convenient concatenation of multiple arrays in a single call.
mvn; that'smvnon the command line by itself.