Add LongestCommonSubstring Impleamentation#7464
Conversation
Rosander0
requested review from
DenizAltunkapan,
alxkm and
yanglbme
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7464 +/- ##
============================================
+ Coverage 79.80% 79.82% +0.02%
- Complexity 7302 7314 +12
============================================
Files 803 804 +1
Lines 23751 23765 +14
Branches 4671 4675 +4
============================================
+ Hits 18955 18971 +16
Misses 4036 4036
+ Partials 760 758 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Rosander0
left a comment
Rosander0
left a comment
There was a problem hiding this comment.
This contians the URL link to documetation explaining the longest common substring algorithm
https://www.geeksforgeeks.org/dsa/longest-common-substring-dp-29/
…ander0/Java-Algorithms into add-longest-common-substring
| */ | ||
| public static String longestCommonSubstring(final String a, final String b) { | ||
| if (a == null || b == null || a.isEmpty() || b.isEmpty()) { | ||
| return ""; |
| } | ||
| } | ||
| } | ||
| return a.substring(endIndex - maxLength, endIndex); |
There was a problem hiding this comment.
When maxLength == 0, this returns "" by coincidence.
Please add explicit check:
if (maxLength == 0) {
return "";
}
return a.substring(endIndex - maxLength, endIndex);
| @Test | ||
| public void testMultipleMatchesFirstLongest() { | ||
| // Keeps the first matched longest substring when lengths are tied | ||
| assertEquals("abc", LongestCommonSubstring.longestCommonSubstring("abcXdef", "abcYdef")); |
There was a problem hiding this comment.
This tie-breaking behavior should also be documented in the
method's Javadoc in implementation file, not just in test comment.
prashantpiyush1111
left a comment
prashantpiyush1111
left a comment
There was a problem hiding this comment.
Hey @Rosander0, good work on the implementation! The DP approach is correct
and test coverage is solid. Just a few things to fix before this can be merged:
1. Reference URL missing from source file
Contributing guidelines require a Wikipedia/reference link directly in the
Javadoc of the implementation file, not just in the PR comments.
@see Wikipedia: Longest Common Substring
2. Fragile edge case
When no common substring exists, the method returns "" by coincidence (substring(0,0)).
Please add an explicit guard:
if (maxLength == 0) {
return "";
}
3. Tie-breaking behavior undocumented
The "first longest wins" contract should be documented in the method's
Javadoc in the implementation file, not just in the test comment.
4. Branch needs cleanup
There are 11 commits with duplicates and unnecessary merge commits:
- "fix: resolve build errors and formatting" appears twice
- "fix: add blank line before imports" appears twice
- 2 unnecessary merge commits
Please squash into 1 clean commit:
git rebase -i HEAD~11
Final commit message should be:
"feat: add LongestCommonSubstring implementation"
5. Minor
Typo in PR title: "Impleamentation" → "Implementation"
Overall the logic is clean, all CI checks are passing and test coverage
is 100%. Fix these small things and this should be good to go!
3 participants
clang-format -i --style=file path/to/your/file.java