London|25-SDC-November|Donara Blanc |Sprint 2| Lru-cache#90
London|25-SDC-November|Donara Blanc |Sprint 2| Lru-cache#90donarbl wants to merge 2 commits intoCodeYourFuture:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
donarbl
added
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
donarbl
changed the title
donarbl
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
To better adhere to the Single-Responsibility Principle (SRP) from SOLID design principles,
it's preferable to implement the "doubly linked list" and the "LRU Cache" as separate classes, with the linked list used inside LruCache to manage ordering.
Alternatively, OrderedDict can be used directly within LruCache to maintain order.
Could you update your code using one of these approaches?
cjyuan
added
Reviewed
refactored by following the recommendation to separate DoublyLinkedList from LruCache
|
Thank you for suggestion and it is done |
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
I don't think the current implementation can pass the tests in lru_cache_test.py.
| self.head = None | ||
| self.tail = None | ||
|
|
||
| def _add_to_head(self, node): |
There was a problem hiding this comment.
What's your reason to to make the methods "private" (by naming convention)?
This could be just a normal linked list.
You could even import the LinkedList you implemented in the other exercise and use it here, and represent the value of data as (key, value).
| new_node = _Node(key, value) | ||
| self._add_to_head(new_node) |
There was a problem hiding this comment.
It would make the LinkedList easier to use if add_to_head() is designed in such a way that the caller can push a new node to the front as:
# Just need to add the data; does not need to concern node creation logic
new_node = self.list.add_to_head((key, value)); # Add key+value as a tuple.
| self._move_to_head(node) | ||
| return node.value | ||
|
|
||
| def set(self, key, value): | ||
| node = self.map.get(key) | ||
|
|
||
| if node is not None: | ||
| # update existing | ||
| node.value = value | ||
| self._move_to_head(node) | ||
| return | ||
|
|
||
| # new key | ||
| if len(self.map) >= self.limit: | ||
| # evict least recently used (tail) | ||
| lru = self.tail | ||
| if lru is not None: | ||
| self._remove_node(lru) | ||
| del self.map[lru.key] | ||
|
|
||
| new_node = _Node(key, value) | ||
| self._add_to_head(new_node) |
There was a problem hiding this comment.
It seems some of the properties and methods accessed here belong to LinkedList (not to LruCache).
2 participants
Change List
I separated DoublyLinkedList from LruCache (SRP) to follos the recommendation of Solid design principle