Solution #1106
Solution #1106
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on building the custom dictionary and implementing the core hashing and collision logic! However, this requires changes to meet two critical requirements. First, your __len__ method has O(n) time complexity, but it must be O(1) as specified in the requirements [CHECKLIST ITEM #1]. This can be fixed by maintaining a size counter in an instance variable. Second, the task requires each stored node to contain key, hash, and value, but your implementation is currently missing the hash. Please update your data structure to include it. Addressing these points will bring your solution in line with the project's specifications.
✨ 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.max_items = self.max_length * 2 // 3 | ||
|
|
||
| def __setitem__(self, key: Any, value: Any) -> None: | ||
| if len(self) >= self.max_items: |
There was a problem hiding this comment.
This check len(self) causes __setitem__ to have O(n) time complexity for every call, due to the inefficient __len__ implementation. According to checklist item #1, this method should be O(1) in the best case. The dictionary should maintain its size in an instance variable to make this check an O(1) operation.
app/main.py
Outdated
| raise KeyError(key) | ||
|
|
||
| def __len__(self) -> int: | ||
| return len([elem for elem in self.my_dict if elem != ""]) |
There was a problem hiding this comment.
This violates checklist item #1 regarding code efficiency. The __len__ method should have a time complexity of O(1). This implementation iterates through the entire internal list, which is O(n). Consider adding an instance variable to track the number of items and updating it when items are added or removed.
app/main.py
Outdated
| break | ||
| index_key = (index_key + 1) % self.max_length | ||
|
|
||
| self.my_dict[index_key] = (key, value) |
There was a problem hiding this comment.
The task description requires each stored item (node) to contain key, hash, and value. This implementation only stores a (key, value) tuple. Storing the pre-calculated hash would prevent recomputing it during operations like lookups or resizing.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on your custom dictionary! You've done a great job implementing the O(1) __len__ method and including the hash in the node structure as requested.
I am requesting one important change before this can be approved. In __setitem__, the dictionary resizes when updating an existing key if the dictionary is at its item limit. This is inefficient, as a resize should only be triggered when a new element is being added that would exceed the load factor. Please adjust the logic to check if the operation is an insert or an update before deciding whether to resize.
✨ 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
|
|
||
| def __setitem__(self, key: Any, value: Any) -> None: | ||
| self.hash_key = hash(key) | ||
| if self.count_items >= self.max_items: |
There was a problem hiding this comment.
This check causes an unnecessary resize when updating an existing key in a dictionary that has reached its item limit. A resize should only be triggered when adding a new element that would exceed the load factor, not during an update.
You should first determine if the operation is an insert or an update before deciding whether to resize.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on the resizing logic! It's a big improvement that __setitem__ now correctly distinguishes between adding and updating items.
I am approving this submission as an educational override to help you move forward, but there is one critical issue to address in your __getitem__ method. You correctly call self.check_item, but you aren't using its boolean return value. This can cause your code to return the wrong value when a key isn't found, instead of raising a KeyError.
To fix this, simply check if self.check_item returns True before accessing the dictionary. If it's False, you should raise the KeyError. You're very close to a complete solution, keep up the great effort!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| def __getitem__(self, key: Any) -> Any: | ||
| try: | ||
| self.index_key = hash(key) % self.max_length | ||
| self.check_item(hash(key), key) | ||
| return self.my_dict[self.index_key][2] | ||
| except IndexError: | ||
| raise KeyError(key) |
There was a problem hiding this comment.
This method has a logic issue. You're correctly calling self.check_item, but you are not using its boolean return value.
If check_item returns False (meaning the key was not found), the code proceeds to return self.my_dict[self.index_key][2]. In some cases, this can return the value of a different key that happens to be at that index, instead of raising a KeyError.
You should check the result of self.check_item and only return a value if it's True. Otherwise, a KeyError should be raised.
2 participants
No description provided.