Merged
Conversation
BrianLusina
added
enhancement
Algorithm
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new MapSum data structure with three alternative implementations (brute-force, prefix-counter, and trie-based) alongside comprehensive tests and documentation. It extends the trie functionality by adding a score attribute to TrieNode and expands existing test coverage whilst updating the directory catalogue. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
📝 Coding Plan
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)
datastructures/map_sum/test_map_sum_pairs.py (1)
60-107: Reduce duplicated operation-runner logic across the three tests.Line 60-Line 107 repeats the same execution/assertion loop three times. Extracting a shared helper keeps these tests easier to maintain.
♻️ Suggested refactor
class MapSumPairsTestCase(unittest.TestCase): + def _assert_operations(self, map_sum, operations: List[Tuple[str, Tuple[str, int]]]) -> None: + for cmd, params in operations: + if cmd == "insert": + key, value = params + map_sum.insert(key, value) + if cmd == "sum": + prefix, expected = params + actual = map_sum.sum(prefix) + self.assertEqual(expected, actual) + `@parameterized.expand`(MAP_SUM_TEST_CASES) def test_map_sum_pairs_brute_force( self, operations: List[Tuple[str, Tuple[str, int]]] ): - map_sum = MapSumBruteForce() - for operation in operations: - cmd = operation[0] - params = operation[1] - if cmd == "insert": - key, value = params - map_sum.insert(key, value) - - if cmd == "sum": - prefix, expected = params - actual = map_sum.sum(prefix) - self.assertEqual(expected, actual) + self._assert_operations(MapSumBruteForce(), operations) `@parameterized.expand`(MAP_SUM_TEST_CASES) def test_map_sum_pairs_prefix(self, operations: List[Tuple[str, Tuple[str, int]]]): - map_sum = MapSumPrefix() - for operation in operations: - cmd = operation[0] - params = operation[1] - if cmd == "insert": - key, value = params - map_sum.insert(key, value) - - if cmd == "sum": - prefix, expected = params - actual = map_sum.sum(prefix) - self.assertEqual(expected, actual) + self._assert_operations(MapSumPrefix(), operations) `@parameterized.expand`(MAP_SUM_TEST_CASES) def test_map_sum_pairs_trie(self, operations: List[Tuple[str, Tuple[str, int]]]): - map_sum = MapSumTrie() - for operation in operations: - cmd = operation[0] - params = operation[1] - if cmd == "insert": - key, value = params - map_sum.insert(key, value) - - if cmd == "sum": - prefix, expected = params - actual = map_sum.sum(prefix) - self.assertEqual(expected, actual) + self._assert_operations(MapSumTrie(), operations)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@datastructures/map_sum/test_map_sum_pairs.py` around lines 60 - 107, Extract the duplicated operation-runner loop into a helper method on MapSumPairsTestCase (e.g., add a private method run_operations(self, map_sum, operations)) that iterates operations, calls insert or sum on the provided map_sum instance and performs the assertEqual checks; then simplify test_map_sum_pairs_brute_force, test_map_sum_pairs_prefix, and test_map_sum_pairs_trie to construct their MapSumBruteForce / MapSumPrefix / MapSumTrie instances and call self.run_operations(map_sum, operations) instead of repeating the loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@datastructures/map_sum/README.md`:
- Around line 15-17: Update the API method signatures in the README to
Python-style for the MapSum methods: replace the Java-style lines for `insert`
and `sum` with Python signatures such as `def insert(self, key: str, val: int)
-> None` and `def sum(self, prefix: str) -> int` (or equivalent PEP484 type
hints used elsewhere), ensuring the method names `insert` and `sum` match the
implemented class (MapSum) and keep the descriptions unchanged.
In `@datastructures/trees/trie/trie_node.py`:
- Around line 10-11: The TrieNode docstring incorrectly states the node
"contains a character"; update the docstring for class TrieNode to describe the
actual attributes: children (dict mapping chars to TrieNode), is_end (bool),
index (optional/int), and score (optional/float), and remove the mention of a
stored character so the description matches the implemented fields (refer to
TrieNode, children, is_end, index, score).
---
Nitpick comments:
In `@datastructures/map_sum/test_map_sum_pairs.py`:
- Around line 60-107: Extract the duplicated operation-runner loop into a helper
method on MapSumPairsTestCase (e.g., add a private method run_operations(self,
map_sum, operations)) that iterates operations, calls insert or sum on the
provided map_sum instance and performs the assertEqual checks; then simplify
test_map_sum_pairs_brute_force, test_map_sum_pairs_prefix, and
test_map_sum_pairs_trie to construct their MapSumBruteForce / MapSumPrefix /
MapSumTrie instances and call self.run_operations(map_sum, operations) instead
of repeating the loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fdeeca73-0ec4-410a-abec-0929f68c2dca
⛔ Files ignored due to path filters (3)
datastructures/map_sum/images/examples/map_sum_pairs_example_1.pngis excluded by!**/*.pngdatastructures/map_sum/images/examples/map_sum_pairs_example_2.pngis excluded by!**/*.pngdatastructures/map_sum/images/examples/map_sum_pairs_example_3.pngis excluded by!**/*.png
📒 Files selected for processing (6)
DIRECTORY.mddatastructures/map_sum/README.mddatastructures/map_sum/__init__.pydatastructures/map_sum/test_map_sum_pairs.pydatastructures/trees/trie/trie_node.pydatastructures/trees/trie/word_dictionary/test_word_dictionary.py
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:
Map Sum Pairs data structure
Checklist:
Fixes: #{$ISSUE_NO}.Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests