Implemented sliding2 Array Operation #53
Conversation
- Implemented a generic method using Vavr's Tuple2 to create adjacent pairs from an input array. - Utilized Guava's ImmutableList to return an immutable list of pairs. - Added null and length checks to handle edge cases (null or array with fewer than 2 elements).
WalkthroughThis pull request updates the project’s dependency configuration by adding new libraries (Guava and Vavr) while noting duplicate test dependencies in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Arrays
Client->>Arrays: sliding2(array)
Note right of Arrays: Validate input (null or <2 elements)
Arrays->>Arrays: Iterate array and form Tuple2 pairs
Arrays->>Client: Return ImmutableList of pairs
Possibly related issues
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/fungover/breeze/util/Arrays.java (1)
20-30: Consider using ImmutableList.Builder for better performance.The current implementation uses an ArrayList as intermediate storage before converting to ImmutableList. Using ImmutableList.Builder directly would be more efficient.
- // List to store pairs of adjacent elements - List<Tuple2<T, T>> pairs = new ArrayList<>(); + // Builder to create the immutable list directly + ImmutableList.Builder<Tuple2<T, T>> builder = ImmutableList.builder(); // Loop through the array and create pairs of adjacent elements for (int i = 0; i < array.length - 1; i++) { - // Create a pair (Tuple2) for the current element and the next element - pairs.add(Tuple.of(array[i], array[i + 1])); + builder.add(Tuple.of(array[i], array[i + 1])); } - // Return the list of pairs wrapped in an immutable list - return ImmutableList.copyOf(pairs); + return builder.build();src/test/java/org/fungover/breeze/util/ArraysTest.java (1)
53-81: Consider improving the performance test.Two suggestions for the performance test:
- The hard-coded 500ms threshold might fail on slower CI environments.
- Using
System.out.printlnfor logging in tests is not ideal.- System.out.println("Large array test completed in " + durationMs + " ms"); + // Use a logging framework or test reporter + logger.info("Large array test completed in {} ms", durationMs); - // Optional: Assert performance (e.g., should complete in < 500ms) - assertTrue(durationMs < 500, "Performance issue: took too long!"); + // Consider using assumptions or configurable thresholds + assumeTrue(durationMs < getConfiguredThreshold(), + "Performance test skipped: environment too slow");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pom.xml(1 hunks)src/main/java/org/fungover/breeze/util/Arrays.java(1 hunks)src/test/java/org/fungover/breeze/util/ArraysTest.java(1 hunks)
🔇 Additional comments (4)
src/main/java/org/fungover/breeze/util/Arrays.java (2)
1-10: LGTM!The class structure and imports are well-organized and appropriate for the functionality.
11-31: LGTM! Well-implemented sliding pairs functionality.The implementation is correct, thread-safe, and handles edge cases appropriately:
- Null check prevents NPE
- Length check ensures sufficient elements
- ImmutableList ensures thread safety
src/test/java/org/fungover/breeze/util/ArraysTest.java (2)
11-30: LGTM! Well-structured test cases.The test cases thoroughly verify the functionality:
- Normal array test checks both size and content
- Two-element array test verifies minimal case
31-51: LGTM! Comprehensive edge case coverage.The test suite effectively covers important edge cases:
- Single element array
- Empty array
- Array with null elements
| <dependency> | ||
| <groupId>org.junit.jupiter</groupId> | ||
| <artifactId>junit-jupiter</artifactId> | ||
| <version>5.10.3</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.assertj</groupId> | ||
| <artifactId>assertj-core</artifactId> | ||
| <version>3.27.3</version> | ||
| </dependency> |
There was a problem hiding this comment.
Remove duplicate dependencies.
The following dependencies are duplicated:
org.junit.jupiter:junit-jupiterorg.assertj:assertj-core
Please remove lines 30-40 to fix this issue.
| <artifactId>junit-jupiter</artifactId> | ||
| <version>5.10.3</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
These are duplicated dependencies. Update the already existing entry if they need a version bump
There was a problem hiding this comment.
Make sure to add Javadoc documentation. It will help developers to have an easier time understanding the class and it's functions
There was a problem hiding this comment.
If possible, add more test cases to reach 100% code coverage. Currently, these tests only cover 84.6%
|
|
||
| import com.google.common.collect.ImmutableList; | ||
| import io.vavr.Tuple; | ||
| import io.vavr.Tuple2; |
There was a problem hiding this comment.
@kappsegla should io.vavr be used or should we use our Tuple2 implementation?
There was a problem hiding this comment.
We should use our own Tuple2. We don't want any 3rd party dependencies. So either wait until we have our own datatype here or create your own datatype as a placeholder and replace it with the actual implementation in the future. Maybe the Tuple2 class is available with enough functionality to be used here? Should only need the of() method to be able to create and return such a class? Do a cherry-pick from #8 branch?
|
Looks good, could also make the constructor private. |
PR for Issue: #20
Merging this PR adds the sliding2 array operation. Key features and improvements implemented in this PR include:
Summary by CodeRabbit