feat(algorithms, trie, index pairs): index pairs of strings#195
Open
BrianLusina wants to merge 2 commits intomainfrom
Open
feat(algorithms, trie, index pairs): index pairs of strings#195BrianLusina wants to merge 2 commits intomainfrom
BrianLusina wants to merge 2 commits intomainfrom
Conversation
BrianLusina
added
enhancement
Algorithm
Contributor
📝 WalkthroughWalkthroughA new Trie-based algorithm is introduced to find all index pairs of words within a text string. The implementation includes the primary algorithm, comprehensive documentation explaining the approach, and test coverage with multiple fixtures validating the solution. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
algorithms/trie/index_pairs_of_a_string/__init__.py (1)
13-13: Nit: unused loop variable.
charfromenumerate(text)is never read inside the outer loop (the inner loop re-reads viatext[j]). Considerfor idx in range(len(text)):to make that explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@algorithms/trie/index_pairs_of_a_string/__init__.py` at line 13, The outer loop currently uses "for idx, char in enumerate(text):" but "char" is never used; change it to a loop that makes intent explicit (e.g., "for idx in range(len(text)):" or "for idx, _ in enumerate(text):") in the function that iterates over the variable text (the outer loop that contains the inner loop accessing text[j]) so the unused loop variable warning is removed and intent is clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@algorithms/trie/index_pairs_of_a_string/__init__.py`:
- Around line 4-31: Add a docstring with a concise description of
index_pairs(text, words) inputs and return semantics and include at least one
doctest example; update the index_pairs function (which uses Trie, trie.insert,
trie.root and node.is_end) to have a top-level triple-quoted docstring that
documents parameters and return value and contains a runnable doctest (for
example using text = "thestoryofleetcodeandme", words =
["story","fleet","leetcode"] expecting [[3,7],[9,13]] or another existing
fixture) so automated tests pick it up.
In `@algorithms/trie/index_pairs_of_a_string/README.md`:
- Around line 44-48: Update the "Time Complexity" paragraph to accurately
separate trie construction and search phases: state total complexity as O(n*m +
l*k) (with worst-case search O(l^2)), where n and m refer to the number/average
length of words used to build the trie and l and k refer to the text length and
average/matched word length respectively; explicitly note that l*k describes the
entire search phase across all start indices (not per-index), and keep the
alternative worst-case O(n*m + l^2) to cover degenerate cases.
---
Nitpick comments:
In `@algorithms/trie/index_pairs_of_a_string/__init__.py`:
- Line 13: The outer loop currently uses "for idx, char in enumerate(text):" but
"char" is never used; change it to a loop that makes intent explicit (e.g., "for
idx in range(len(text)):" or "for idx, _ in enumerate(text):") in the function
that iterates over the variable text (the outer loop that contains the inner
loop accessing text[j]) so the unused loop variable warning is removed and
intent is clearer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15576026-591f-4789-98ee-b17a74d651f6
⛔ Files ignored due to path filters (3)
algorithms/trie/index_pairs_of_a_string/images/examples/index_pairs_of_a_string_example_1.pngis excluded by!**/*.pngalgorithms/trie/index_pairs_of_a_string/images/examples/index_pairs_of_a_string_example_2.pngis excluded by!**/*.pngalgorithms/trie/index_pairs_of_a_string/images/examples/index_pairs_of_a_string_example_3.pngis excluded by!**/*.png
📒 Files selected for processing (4)
DIRECTORY.mdalgorithms/trie/index_pairs_of_a_string/README.mdalgorithms/trie/index_pairs_of_a_string/__init__.pyalgorithms/trie/index_pairs_of_a_string/test_index_pairs_of_a_string.py
Comment on lines
+4
to
+31
| def index_pairs(text: str, words: List[str]) -> List[List[int]]: | ||
| trie = Trie() | ||
|
|
||
| for word in words: | ||
| trie.insert(word) | ||
|
|
||
| results = [] | ||
|
|
||
| # loop through each character in the text | ||
| for idx, char in enumerate(text): | ||
| # start from the root of the Trie for each character in the text | ||
| node = trie.root | ||
|
|
||
| # Check each possible substring starting from index idx | ||
| for j in range(idx, len(text)): | ||
| ch = text[j] | ||
| # If the character is not in the current Trie Node's children, stop searching | ||
| if ch not in node.children: | ||
| break | ||
|
|
||
| # Move to the next node in the Trie | ||
| node = node.children[ch] | ||
|
|
||
| # If we reach the end of a word, record the indices | ||
| if node.is_end: | ||
| results.append([idx, j]) | ||
|
|
||
| return results |
Contributor
There was a problem hiding this comment.
Missing docstring/doctest on the public function.
The PR checklist commits to "All functions include doctests that pass automated testing", but index_pairs has neither a docstring nor doctests. Please add a brief docstring describing text/words/return semantics and at least one doctest (e.g. reusing one of the test fixtures) so it conforms to the repository's contribution guideline.
📝 Suggested docstring with doctest
def index_pairs(text: str, words: List[str]) -> List[List[int]]:
+ """
+ Returns all index pairs [i, j] such that text[i..j] (inclusive) is in words,
+ sorted by i then by j.
+
+ >>> index_pairs("howareyou", ["how", "are", "you"])
+ [[0, 2], [3, 5], [6, 8]]
+ >>> index_pairs("xyxyx", ["xyx", "xy"])
+ [[0, 1], [0, 2], [2, 3], [2, 4]]
+ """
trie = Trie()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@algorithms/trie/index_pairs_of_a_string/__init__.py` around lines 4 - 31, Add
a docstring with a concise description of index_pairs(text, words) inputs and
return semantics and include at least one doctest example; update the
index_pairs function (which uses Trie, trie.insert, trie.root and node.is_end)
to have a top-level triple-quoted docstring that documents parameters and return
value and contains a runnable doctest (for example using text =
"thestoryofleetcodeandme", words = ["story","fleet","leetcode"] expecting
[[3,7],[9,13]] or another existing fixture) so automated tests pick it up.
Comment on lines
+44
to
+48
| ### Time Complexity | ||
|
|
||
| Inserting n words of average length m into the trie takes O(n∗m). For each index i in text, we perform a search that | ||
| takes linear time in the length of the substring. This gives an overall time complexity of O(l∗k), where l is the length | ||
| of the text, and k is the average length of a word. |
Contributor
There was a problem hiding this comment.
Time complexity statement is inconsistent/under-stated.
The stated complexity O(l*k) ignores the outer loop over every start index in text. The algorithm performs, for each of the l starting positions, a trie walk of up to k (or up to l) characters, yielding O(l*k) or O(l^2) for the search phase — plus O(n*m) for trie construction. As written, the expression conflates the per-start-index traversal cost with the total search cost. Consider stating it as O(n*m + l*k) total (or O(n*m + l^2) worst case) and clarifying that l*k is the search phase, not a per-index cost.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~46-~46: Possible missing article found.
Context: ... trie takes O(n∗m). For each index i in text, we perform a search that takes linear ...
(AI_HYDRA_LEO_MISSING_THE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@algorithms/trie/index_pairs_of_a_string/README.md` around lines 44 - 48,
Update the "Time Complexity" paragraph to accurately separate trie construction
and search phases: state total complexity as O(n*m + l*k) (with worst-case
search O(l^2)), where n and m refer to the number/average length of words used
to build the trie and l and k refer to the text length and average/matched word
length respectively; explicitly note that l*k describes the entire search phase
across all start indices (not per-index), and keep the alternative worst-case
O(n*m + l^2) to cover degenerate cases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your change:
Index Pairs of strings
Checklist:
Fixes: #{$ISSUE_NO}.Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests