-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Solution #1099
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Solution #1099
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
I am requesting changes because there are still two checklist violations that are considered core requirements, not just style preferences. First, the checklist explicitly requires that any method adding or deleting an item must update self.length, but your implementation uses self.size instead; you should rename the attribute to self.length (initialized where self.size currently is) and update all its uses, including increments, resets during resize, and in __len__ so that __len__ returns self.length [CHECKLIST ITEM #3]. Second, on line 70 you raise KeyError(key) without a descriptive message; the checklist requires providing a message when raising exceptions, so change this to include context, e.g. raise KeyError(f"Key not found: {key}") or similar [CHECKLIST ITEM #1]. Once those are fixed, your Dictionary implementation already meets the functional requirements (Node structure, chaining, __setitem__, __getitem__, __len__, and resizing), so no further mandatory changes are needed. Optional enhancements like adding more dictionary-like methods (clear, get, pop, etc.) and extra tests for collisions/resizing could further strengthen your solution but are not blockers. Overall, you’re very close—addressing the length attribute naming and the KeyError message will bring your code fully in line with the checklist and task requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
app/main.py
Outdated
| self.size: int = 0 | ||
| self.table: List[Optional[Node]] = [None] * self.capacity | ||
|
|
||
| def __len__(self) -> int: | ||
| return self.size | ||
|
|
||
| def _get_index(self, key_hash: int) -> int: | ||
| return key_hash % self.capacity | ||
|
|
||
| def __setitem__(self, key: Any, value: Any) -> None: | ||
| # Resize if needed | ||
| if self.size + 1 > self.capacity * self.load_factor: | ||
| self._resize() | ||
|
|
||
| key_hash = hash(key) | ||
| index = self._get_index(key_hash) | ||
| node = self.table[index] | ||
|
|
||
| # No collision → save directly | ||
| if node is None: | ||
| self.table[index] = Node(key, value) | ||
| self.size += 1 | ||
| return | ||
|
|
||
| # Collision → walk linked list | ||
| prev: Optional[Node] = None | ||
| while node: | ||
| if node.key == key: | ||
| node.value = value | ||
| return | ||
| prev = node | ||
| node = node.next | ||
|
|
||
| # Add new node at end | ||
| prev.next = Node(key, value) | ||
| self.size += 1 | ||
|
|
||
| def __getitem__(self, key: Any) -> Any: | ||
| key_hash = hash(key) | ||
| index = self._get_index(key_hash) | ||
| node = self.table[index] | ||
|
|
||
| while node: | ||
| if node.key == key: | ||
| return node.value | ||
| node = node.next | ||
|
|
||
| raise KeyError(key) | ||
|
|
||
| def _resize(self) -> None: | ||
| """Double table size and rehash all items.""" | ||
| old_table = self.table | ||
| self.capacity *= 2 | ||
| self.table = [None] * self.capacity | ||
| self.size = 0 # will re-add all nodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This violates the checklist requirement: "in any method that add or delete an item we change self.length" (from the Good example) . The class uses the attribute self.size (initialized on line 23 and updated elsewhere) instead of the expected self.length. Please rename the counter to self.length and update all uses (initialization, increments, resets during resize, and len) so the stored length attribute matches the checklist example and __len__ returns that stored attribute.
app/main.py
Outdated
| return node.value | ||
| node = node.next | ||
|
|
||
| raise KeyError(key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This violates the checklist requirement: "Make sure that you provide a message when an exception raises." . On line 70 the code raises KeyError(key) with no descriptive message. Provide a clearer exception message, for example: raise KeyError(f"Key not found: {key}"), so users/tests get an informative message.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
I am approving your solution because it cleanly meets all of the core requirements for the custom Dictionary: you implemented a separate Node that stores key, hash, and value, used a list-based hash table with chaining, and correctly provided __setitem__, __getitem__, and __len__ with proper length tracking and descriptive KeyErrors. The resize behavior with an initial capacity and load factor is in place, and collision handling via linked lists per bucket is correctly implemented, so the fundamental functionality is solid and aligned with the checklist.
One area you might improve later is the efficiency of _resize: instead of calling __setitem__ for each node (which can trigger further resizes), you can reinsert directly into the new buckets using node.hash % new_capacity and temporarily skip load-factor checks. You can also make small optimizations by reusing the stored node.hash instead of recomputing hash(key) when moving existing entries. As optional enhancements, consider adding more dict-like methods such as clear, get, pop, __delitem__, update, and __iter__, and write tests that explicitly cover collisions and resizing behavior. Overall, your implementation demonstrates a solid understanding of hash tables and meets the required specification—nice work.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.