Repository navigation
fix(searches): support duplicates and null safety in LowerBound and UpperBound - #7648
Conversation
When multiple elements equal to key exist, LowerBound should return the index of the first such value per its specification. Continue searching the left partition when comp <= 0 instead of prematurely returning the median element.
…rBound Guard against NullPointerException and empty array bounds access in LowerBound and UpperBound find methods when array is null/empty or key is null.
…ll safety Verify LowerBound returns the first occurrence for duplicate elements, and handles empty arrays, null arrays, and null keys gracefully in LowerBound and UpperBound.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
Hi @alxkm ! 👋 This PR is ready for review:
Thank you! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7648 +/- ##
============================================
+ Coverage 82.17% 82.19% +0.01%
- Complexity 8367 8375 +8
============================================
Files 842 842
Lines 26093 26095 +2
Branches 5090 5089 -1
============================================
+ Hits 21443 21448 +5
Misses 3858 3858
+ Partials 792 789 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alxkm
left a comment
There was a problem hiding this comment.
Thanks! The duplicate fix in LowerBound looks correct, and the tests cover it well.
Three small things:
- Returning
0for anullarray/key silently hides caller errors. WouldObjects.requireNonNull(...)(withassertThrowsin tests) be better? If we keep0, please document it in@return. - Without the early return,
LowerBound's best case is no longer O(1). Please update the Javadoc. - Could you add a duplicates test for
UpperBoundtoo (e.g.{1, 2, 2, 2, 3}, key2→4)?
…exity, and test UpperBound duplicates
81c661e to
4d50b7f
Compare
|
Hi @alxkm, Thank you for the review feedback! I have updated the PR to address all three points. |
alxkm
left a comment
There was a problem hiding this comment.
Looks good. Thank you for the contribution.
Description
This pull request addresses two issues in
LowerBoundandUpperBound:LowerBound:The Javadoc for
LowerBoundspecifies: "If there are multiple values that are equal to val it returns the index of the first such value."Previously, when
comp == 0,search()immediately returnedmedian. When an array contained duplicates (e.g.,[1, 2, 2, 2, 3]), this returned an intermediate median index instead of the first occurrence. Changing the condition toif (comp <= 0)ensures the search continues exploring the left half to find the first matching element index.Added edge case checks in
LowerBound.find()andUpperBound.find()to return0whenarray == null,array.length == 0, orkey == null, preventingNullPointerExceptionand unexpected behavior on null/empty inputs.Added tests in
LowerBoundTestcovering duplicate elements, all-identical arrays, empty arrays, null arrays, and null keys, along with null checks inUpperBoundTest.clang-format -i --style=file path/to/your/file.java