implemented task #1117
implemented task #1117
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 a functional custom Dictionary class! The core logic for setting, getting, and resizing is well-implemented. However, a couple of changes are needed to meet all the project requirements.
First, the task requires each node to store the key, hash, and value. Your current implementation is missing the hash in the stored items. Please update your node structure to include it.
Second, when raising a KeyError, you need to provide a descriptive message, as stated in the checklist. For example: raise KeyError(f'Key not found: {key}').
Once these two requirements are met, your solution will be ready for approval. Keep up the great work!
✨ 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
| @@ -1,2 +1,104 @@ | |||
| from itertools import count | |||
There was a problem hiding this comment.
The itertools.count function is imported but never used in the file. Unused imports should be removed to keep the code clean.
app/main.py
Outdated
| has_collision = False | ||
| collision_index = 0 | ||
| node = self.hash_table[index] | ||
| item_to_add = (key, value) |
There was a problem hiding this comment.
The task description requires each node to store key, hash, and value. The current implementation only stores (key, value). Please update your node structure to include the hash, which will require adjustments where items are created and accessed.
app/main.py
Outdated
| key, value = item | ||
| if key == item_key: | ||
| return value | ||
| raise KeyError |
There was a problem hiding this comment.
This violates a checklist item: 'Make sure that you provide a message when an exception raises.' Please add a descriptive message to the KeyError, for example: raise KeyError(f'Key not found: {item_key}').
app/main.py
Outdated
| pop_value = self.__getitem__(key) | ||
| item_to_delete = (key, pop_value) | ||
| if item_to_delete not in node: | ||
| raise KeyError |
There was a problem hiding this comment.
This block has some redundant logic. The call to self.__getitem__(key) on line 62 already confirms the key exists and finds its value. Therefore, the check if item_to_delete not in node: on line 64 is unnecessary and the raise on line 65 is unreachable. This implementation also results in traversing the bucket list multiple times (in __getitem__, the in check, and remove). Consider refactoring to find and remove the item in a single loop for better efficiency.
tests/test_main.py
Outdated
| assert len(dictionary) == len(items) | ||
| for key, value in items: | ||
| del dictionary[key] | ||
| print(len(dictionary)) |
There was a problem hiding this comment.
This print statement appears to be for debugging purposes. According to the checklist, temporary checks and prints should be removed before submitting the final code to ensure it's clean.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on implementing the core functionality of the Dictionary class. However, a couple of key requirements from the task description and checklist are still pending, so I am requesting a few changes.
First, each node in the hash table must store the key, hash, and value. Your current implementation stores items as (key, value) tuples. Please update your data structure to include the pre-computed hash and adjust the methods that interact with it.
Second, the KeyError exceptions raised in __getitem__ and __delitem__ need a descriptive message, as stated in the checklist. For example, you could use raise KeyError(f"Key not found: {key}").
Lastly, please remove the debugging print statement from tests/test_main.py to keep the code clean. Once these items are addressed, your solution will be complete!
✨ 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
| has_collision = False | ||
| collision_index = 0 | ||
| node = self.hash_table[index] | ||
| item_to_add = (key, value) |
There was a problem hiding this comment.
According to the task description, each node in the hash table should store the key, hash, and value. This tuple only contains the key and value. You'll need to include the pre-computed hash of the key and update other parts of the code that handle these nodes.
app/main.py
Outdated
| key, value = item | ||
| if key == item_key: | ||
| return value | ||
| raise KeyError |
There was a problem hiding this comment.
This violates the checklist item under Code Style: "Make sure that you provide a message when an exception raises." Please add a descriptive message to this KeyError.
app/main.py
Outdated
| pop_value = self.__getitem__(key) | ||
| item_to_delete = (key, pop_value) | ||
| if item_to_delete not in node: | ||
| raise KeyError |
There was a problem hiding this comment.
This violates the checklist item under Code Style: "Make sure that you provide a message when an exception raises." Please add a descriptive message to this KeyError.
tests/test_main.py
Outdated
| assert len(dictionary) == len(items) | ||
| for key, value in items: | ||
| del dictionary[key] | ||
| print(len(dictionary)) |
There was a problem hiding this comment.
This print statement should be removed. The 'Clean Code' section of the checklist requires removing temporary prints, comments, and check functions before submitting the code.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this submission! I am approving your solution as you've successfully addressed all the feedback from the previous review. The implementation of the Dictionary class is now fully compliant with all requirements, including the node structure and descriptive exception messages. The logic for handling hashing, collisions, and resizing is solid. Great job completing the task!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.