-
Notifications
You must be signed in to change notification settings - Fork 20.9k
Add BinarySearchStrings algorithm with comprehensive tests #7221
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?
Add BinarySearchStrings algorithm with comprehensive tests #7221
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7221 +/- ##
============================================
+ Coverage 79.05% 79.07% +0.01%
- Complexity 6946 6957 +11
============================================
Files 779 780 +1
Lines 22908 22937 +29
Branches 4502 4508 +6
============================================
+ Hits 18110 18137 +27
- Misses 4077 4078 +1
- Partials 721 722 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
singhc7
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.
The clang-format check is failing, probably just some whitespace or indentation issues. Run clang-format command on your files to auto-fix the style.
Also, left some comments on the implementation.
|
|
||
| while (left <= right) { | ||
| int mid = left + (right - left) / 2; | ||
| int comparison = targetLower.compareTo(array[mid].toLowerCase()); |
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.
array[mid].toLowerCase() allocates a new String object every single iteration. That's heavy on memory.
You should just use target.compareToIgnoreCase(array[mid]) here instead.
| * @param target the string to search for | ||
| * @return index of target string if found, -1 otherwise | ||
| */ | ||
| public static int searchIgnoreCase(String[] array, String target) { |
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.
The logic here is 100% identical to the search method above.
Please refactor this to use a private helper method that accepts a Comparator. That way you don't duplicate the binary search code twice.
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.
@singhc7 Thank you for the detailed review! You're absolutely right on all points:
-
✅ Memory optimization: Replaced
targetLower.compareTo(array[mid].toLowerCase())withtarget.compareToIgnoreCase(array[mid])to avoid unnecessary String object creation. -
✅ Code deduplication: Refactored both methods to use a private
binarySearch(String[] array, String target, Comparator<String> comparator)helper method. Nowsearch()usesString::compareToandsearchIgnoreCase()usesString::compareToIgnoreCase. -
✅ Formatting: Will run clang-format before the next push.
The refactored code is much cleaner and more efficient. Thanks for the guidance!
clang-format -i --style=file path/to/your/file.java