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 implementing the core functionality of MyHashMap! The structure is solid, but there are a few critical issues that need to be addressed. Changes are needed for the following points:
resize()Bug: Theresize()method doesn't correctly handle an entry with anullkey, which leads to an incorrectsizecount after the table is resized.- Potential
ArrayIndexOutOfBoundsException: In yourgetIndex()method,Math.abs(key.hashCode())can return a negative number if the hash code isInteger.MIN_VALUE. This will cause a crash. A safer approach is to use a bitwise AND to ensure the value is non-negative, for example:(key.hashCode() & 0x7fffffff) % capacity. - Checklist Violation: The inner
Nodeclass contains getters and setters, which violates a specific project requirement [CHECKLIST ITEM #3]. You should access the node's fields directly (e.g.,someNode.key) from within theMyHashMapclass.
Please fix these issues, and I'll be happy to review your work again!
✨ 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 void resize() { | ||
| Node<K, V>[] oldTable = table; | ||
| table = new Node[oldTable.length * 2]; | ||
| size = 0; |
There was a problem hiding this comment.
Resetting size to 0 here is incorrect if an entry with a null key exists. The for loop below only re-inserts elements from the table, but the null-keyed entry is stored separately and its count is lost. The size should be initialized based on whether hasNullKey is true before re-inserting the other elements.
| K getKey() { | ||
| return key; | ||
| } | ||
|
|
||
| V getValue() { | ||
| return value; | ||
| } | ||
|
|
||
| void setValue(V value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| Node<K, V> getNext() { | ||
| return next; | ||
| } | ||
|
|
||
| void setNext(Node<K, V> next) { | ||
| this.next = next; | ||
| } |
There was a problem hiding this comment.
This violates checklist item #3: 'Don't use getters and setters in class Node'. Since Node is a private inner class, the MyHashMap class can access its private fields directly (e.g., someNode.key or someNode.value = newValue). Using direct field access is more efficient and simplifies the Node class.
| } | ||
|
|
||
| private int getIndex(K key, int capacity) { | ||
| return Math.abs(key.hashCode()) % capacity; |
There was a problem hiding this comment.
Using Math.abs() on a hash code can be risky. If key.hashCode() returns Integer.MIN_VALUE, Math.abs() will also return Integer.MIN_VALUE (which is negative). This will result in a negative index and an ArrayIndexOutOfBoundsException. A safer approach is to use a bitwise AND to clear the sign bit, like (key.hashCode() & 0x7fffffff) % capacity.
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 critical issues from the last review! Your MyHashMap is much more robust now, and the solution is very close to complete. Before this can be approved, a few minor adjustments are needed to fully align with the project's checklist.
- The resizing factor
2in theresize()method is a magic number. Please declare it as a named constant, for example,private static final int RESIZE_MULTIPLIER = 2;. [CHECKLIST ITEM #2] - The constructor for the inner
Nodeclass is missing an access modifier. Since it's only used withinMyHashMap, it should be declaredprivate. [CHECKLIST ITEM #7] - Please remove the redundant empty lines between the field declarations to group them together. [CHECKLIST ITEM #1]
Once these small items are addressed, your implementation will be in excellent shape. 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
| public class MyHashMap<K, V> implements MyMap<K, V> { | ||
|
|
||
| private static final int DEFAULT_CAPACITY = 16; | ||
|
|
There was a problem hiding this comment.
This violates checklist item #1: 'Remove all redundant empty lines'. The empty lines between each field declaration are redundant. It would be better to group these fields together.
|
|
||
| private void resize() { | ||
| Node<K, V>[] oldTable = table; | ||
| table = new Node[oldTable.length * 2]; |
There was a problem hiding this comment.
This violates checklist item #2: 'Any magic numbers should be constants'. The resizing factor 2 should be declared as a named constant to improve readability and maintainability, for example: private static final int RESIZE_MULTIPLIER = 2;.
| private V value; | ||
| private Node<K, V> next; | ||
|
|
||
| Node(K key, V value) { |
There was a problem hiding this comment.
This violates checklist item #7: 'All fields and methods should have access modifiers'. The constructor for the inner Node class should have an explicit access modifier. Since it's only used within MyHashMap, making it private would be appropriate.
# Conflicts: # src/main/java/core/basesyntax/MyHashMap.java
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 iteration! You've successfully addressed all the previous feedback, and your HashMap logic for handling resizing and null keys is solid.
I am approving your solution as an educational override to help you progress, but there is one minor style issue left to fix. In MyHashMap.java, please remove the blank line immediately after the class declaration. This violates the style guide which states not to begin a class or method implementation with an empty line [CHECKLIST ITEM #1].
This is a very small detail in an otherwise great submission. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| public class MyHashMap<K, V> implements MyMap<K, V> { | ||
|
|
||
| private static final int DEFAULT_CAPACITY = 16; |
There was a problem hiding this comment.
This violates checklist item #1: 'Don't begin class or method implementation with empty line'. Please remove the blank line just after the class declaration on the line above this one.
3 participants
No description provided.