done#1683
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation of the HashMap has the correct core logic for put, getValue, and resizing, which is a great start! However, a few adjustments are needed to meet the project's coding standards before this can be approved.
Please address the following points:
- The
resize()method is an internal helper function and should beprivateto properly enforce encapsulation, as required by [CHECKLIST ITEM #6]. - The hardcoded number
2in theresize()method is a magic number. Please define it as a named constant, for exampleprivate static final int RESIZE_MULTIPLIER = 2;, to improve readability [CHECKLIST ITEM #2]. - According to the project's style guide, private methods should be placed after all public methods in the class. Please reorder your methods to comply with this [CHECKLIST ITEM #5].
Once these changes are made, your solution will be ready!
✨ 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
| table = (Node<K, V>[]) new Node[DEFAULT_CAPACITY]; | ||
| } | ||
|
|
||
| public void resize() { |
There was a problem hiding this comment.
This method has a utility purpose and is only used inside this class. According to the encapsulation principle and checklist item #6, it should be private.
|
|
||
| public void resize() { | ||
| Node<K, V>[] oldTable = table; | ||
| table = (Node<K, V>[]) new Node[oldTable.length * 2]; |
There was a problem hiding this comment.
This hardcoded 2 is a magic number. It's better to replace it with a named constant like RESIZE_MULTIPLIER to improve readability and maintainability, as suggested by checklist item #2.
| private int getIndex(K key, int length) { | ||
| if (key == null) { | ||
| return 0; | ||
| } | ||
| return Math.abs(key.hashCode()) % length; | ||
| } | ||
|
|
||
| private boolean keysAreEqual(K firstKey, K secondKey) { | ||
| if (firstKey == null && secondKey == null) { | ||
| return true; | ||
| } | ||
| if (firstKey == null || secondKey == null) { | ||
| return false; | ||
| } | ||
| return firstKey.equals(secondKey); | ||
| } |
There was a problem hiding this comment.
This violates checklist item #5: 'Private methods and classes should be after public ones in your class.' Please move the private helper methods (getIndex, keysAreEqual) to be after all the public methods in this class.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the previous feedback! Your MyHashMap implementation is solid, and you've correctly handled the resizing logic and constants.
However, one small change is needed to fully align with the project's encapsulation principles. The constructor for your private inner Node class is currently public. Since the Node class is private and only used within MyHashMap, its constructor does not need to be public. This violates the principle of least privilege. [CHECKLIST ITEM #6]
Please remove the public access modifier from the Node constructor to give it default (package-private) access. After this quick fix, your solution will be ready for approval!
✨ 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
| private K key; | ||
| private Node<K, V> next; | ||
|
|
||
| public Node(K key, V value) { |
There was a problem hiding this comment.
This constructor is only used within the MyHashMap class. According to checklist item #6, methods (and by extension, constructors) with only a utility purpose inside the same class should not be public. Consider using a more restrictive access modifier to better follow the principle of encapsulation.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on refining your MyHashMap implementation! You've successfully addressed the feedback from the previous review by adjusting the access modifier for the Node constructor. Your code is now fully compliant with all requirements and demonstrates a strong understanding of data structures and encapsulation. The implementation is clean and efficient, so I am approving your solution. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.