refactor(datastructures, lru-cache): refactor lru cache #167
Conversation
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
BrianLusina
added
enhancement
Algorithm
Contributor
📝 WalkthroughWalkthroughAdded LFU cache README; reformatted and corrected LRU README; refactored LRU implementation to use sentinel DoubleNode-based doubly linked list, added/expanded type annotations, and changed 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
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
datastructures/lfucache/README.md (1)
14-14: Typo: missing "t" in "the"."he least recently used" should be "the least recently used".
📝 Suggested fix
-he least recently used key would be invalidated. +the least recently used key would be invalidated.
🤖 Fix all issues with AI agents
In `@datastructures/lrucache/README.md`:
- Line 6: Fix the typo in the README by changing "cahing" to "caching" in the
datastructures/lrucache/README.md line that currently reads "We are cahing the
results of web queries" so the sentence correctly reads "We are caching the
results of web queries".
In `@datastructures/lrucache/with_internal_linked_list.py`:
- Around line 11-14: The constructor sets up sentinel nodes using DoubleNode but
incorrectly assigns self.tail.prev instead of the DoubleNode attribute name
previous; update the initialization to use self.head.next = self.tail and
self.tail.previous = self.head (and ensure any other uses of prev are changed to
previous) so the linked-list sentinel wiring matches the DoubleNode API (inspect
DoubleNode, head, tail, and any methods relying on previous).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
github-actions
bot
force-pushed
the
feat/datastructures-lru-cache
branch
from
bd733b2 to
991c2a7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@datastructures/lrucache/with_internal_linked_list.py`:
- Around line 5-14: The constructor (__init__) doesn't guard against capacity <=
0 which allows put() to trigger eviction of sentinel nodes (tail.previous) and
crash; update the __init__ (the constructor that sets self.capacity,
self.lookup, self.head, self.tail and uses DoubleNode) to validate capacity
(e.g., raise ValueError for capacity < 1 or coerce to a minimum of 1) so
negative or zero capacities are rejected before any put()/eviction logic touches
head/tail/lookup.
🧹 Nitpick comments (1)
datastructures/lrucache/with_internal_linked_list.py (1)
36-51: Alignput()value type withget()return type.
get()returnsOptional[Any], butput()restrictsvaluetoint. If the cache is intended to be generic (as suggested byOptional[Any]andDoubleNode.data: Any), consider widening the type.♻️ Suggested tweak
-def put(self, key: str | int, value: int) -> None: +def put(self, key: str | int, value: Any) -> None:
Comment on lines
5
to
+14
| def __init__(self, capacity: int): | ||
| self.capacity = capacity | ||
| self.lookup = {} | ||
| self.lookup: Dict[str | int, DoubleNode] = {} | ||
| self.size = 0 | ||
| self.head = Node() | ||
| self.tail = Node() | ||
| # Using sentinel head and tail nodes avoids null checks when adding/removing nodes at boundaries. This means | ||
| # every real node always has non-null prev/next pointers, simplifying insertion and deletion logic dramatically | ||
| self.head = DoubleNode(0) | ||
| self.tail = DoubleNode(0) | ||
| self.head.next = self.tail | ||
| self.tail.prev = self.head | ||
| self.tail.previous = self.head |
Contributor
There was a problem hiding this comment.
Guard against zero/negative capacity to avoid sentinel eviction.
With capacity <= 0, put() falls into eviction logic and attempts to delete the sentinel (tail.previous), which can raise KeyError/AttributeError. A simple validation in __init__ avoids this class of bugs.
🛠️ Suggested fix
def __init__(self, capacity: int):
+ if capacity <= 0:
+ raise ValueError("capacity must be a positive integer")
self.capacity = capacity
self.lookup: Dict[str | int, DoubleNode] = {}
self.size = 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def __init__(self, capacity: int): | |
| self.capacity = capacity | |
| self.lookup = {} | |
| self.lookup: Dict[str | int, DoubleNode] = {} | |
| self.size = 0 | |
| self.head = Node() | |
| self.tail = Node() | |
| # Using sentinel head and tail nodes avoids null checks when adding/removing nodes at boundaries. This means | |
| # every real node always has non-null prev/next pointers, simplifying insertion and deletion logic dramatically | |
| self.head = DoubleNode(0) | |
| self.tail = DoubleNode(0) | |
| self.head.next = self.tail | |
| self.tail.prev = self.head | |
| self.tail.previous = self.head | |
| def __init__(self, capacity: int): | |
| if capacity <= 0: | |
| raise ValueError("capacity must be a positive integer") | |
| self.capacity = capacity | |
| self.lookup: Dict[str | int, DoubleNode] = {} | |
| self.size = 0 | |
| # Using sentinel head and tail nodes avoids null checks when adding/removing nodes at boundaries. This means | |
| # every real node always has non-null prev/next pointers, simplifying insertion and deletion logic dramatically | |
| self.head = DoubleNode(0) | |
| self.tail = DoubleNode(0) | |
| self.head.next = self.tail | |
| self.tail.previous = self.head |
🤖 Prompt for AI Agents
In `@datastructures/lrucache/with_internal_linked_list.py` around lines 5 - 14,
The constructor (__init__) doesn't guard against capacity <= 0 which allows
put() to trigger eviction of sentinel nodes (tail.previous) and crash; update
the __init__ (the constructor that sets self.capacity, self.lookup, self.head,
self.tail and uses DoubleNode) to validate capacity (e.g., raise ValueError for
capacity < 1 or coerce to a minimum of 1) so negative or zero capacities are
rejected before any put()/eviction logic touches head/tail/lookup.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
LRU Cache implementation
Checklist:
Fixes: #{$ISSUE_NO}.Summary by CodeRabbit
Documentation
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.