Add LongestCommonSubstring Implementation#7464
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7464 +/- ##
============================================
+ Coverage 79.80% 79.83% +0.03%
- Complexity 7302 7314 +12
============================================
Files 803 804 +1
Lines 23751 23767 +16
Branches 4671 4676 +5
============================================
+ Hits 18955 18975 +20
+ Misses 4036 4035 -1
+ Partials 760 757 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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/
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!
64bfecb to
9b9bfd7
Compare
prashantpiyush1111
left a comment
There was a problem hiding this comment.
All requested changes have been addressed — reference URL added in Javadoc, explicit maxLength == 0 guard added, and tie-breaking behavior documented in method's Javadoc. LGTM!
clang-format -i --style=file path/to/your/file.java