implement custom hashmap class #1604
Conversation
There was a problem hiding this comment.
Review Summary
I'm requesting changes because there's a critical bug in your hashing logic that can cause the application to crash, along with a few other required fixes.
High Priority:
- Your
hash()method's use ofMath.abs(key.hashCode())will return a negative number if the key's hash code isInteger.MIN_VALUE. This will lead to anArrayIndexOutOfBoundsException. A safer way to ensure a non-negative hash is to use a bitwise AND operation, for example:key.hashCode() & 0x7fffffff.
Other Required Changes:
- In the
resizemethod, you are recalculating the hash for each node instead of using the hash already stored innode.hash. Please use the existing hash to determine the node's new position. - The constructor for the private inner
Nodeclass should not bepublic, as this violates encapsulation principles (Checklist #7). - Please consistently use the diamond operator (
<>) when creating newNodeinstances to adhere to modern Java practices (Checklist #8).
You've built a solid structure for the hash map, and with these fixes, it will be much more robust. 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
| Node<K, V> currentNode = this.getNode(key); | ||
|
|
||
| if (currentNode == null) { | ||
| this.elements[position] = new Node<K, V>(hash, key, value, null); |
There was a problem hiding this comment.
This violates checklist item #8: 'Diamond operator'. The diamond operator <> should be used here for conciseness. The compiler can infer the generic types from the context.
For example:
this.elements[position] = new Node<>(hash, key, value, null);
| currentNode = currentNode.next; | ||
| } | ||
|
|
||
| lastNode.next = new Node<K, V>(hash, key, value, null); |
There was a problem hiding this comment.
This violates checklist item #8: 'Diamond operator'. The diamond operator <> should be used here for conciseness, as the compiler can infer the types.
For example:
lastNode.next = new Node<>(hash, key, value, null);
| int newNodeHash = this.hash(node.key); | ||
| int newPosition = newNodeHash % newCap; |
There was a problem hiding this comment.
There's an inconsistency and inefficiency in how the hash is handled here.
- Inefficiency: You are recalculating the hash for
node.keyon line 77, but the hash is already stored innode.hash. - Inconsistency: You use the newly calculated
newNodeHashto find thenewPosition(line 78), but you use the originalnode.hashwhen creating thenewNode(line 79).
You should not recalculate the hash. Instead, use the hash that is already stored in the node to determine its new position in the resized array.
Consider removing line 77 and changing line 78 to use node.hash.
| } | ||
|
|
||
| private int hash(K key) { | ||
| return key == null ? 0 : Math.abs(key.hashCode()); |
There was a problem hiding this comment.
This hash function has a subtle but important bug. The hashCode() method can return Integer.MIN_VALUE. When this happens, Math.abs(Integer.MIN_VALUE) also returns Integer.MIN_VALUE, which is a negative number.
While your current implementation with power-of-two capacities doesn't crash (because Integer.MIN_VALUE % powerOfTwo happens to be 0), this is not a robust solution. It causes all keys with this specific hash code to cluster in a single bucket, degrading performance. It would also cause an ArrayIndexOutOfBoundsException if the capacity were not a power of two.
A safer way to ensure a non-negative hash is to use a bitwise AND operation, for example: key.hashCode() & 0x7fffffff.
| private V value; | ||
| private Node<K, V> next; | ||
|
|
||
| public Node(int hash, K key, V value, Node<K, V> node) { |
There was a problem hiding this comment.
This violates the encapsulation principle mentioned in checklist items #6 and #7. Since the Node class is private and only used within MyHashMap, its constructor does not need to be public. Changing the access modifier to private or package-private (default) would be more appropriate and improve encapsulation.
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! Your MyHashMap is functionally correct and meets all the core requirements of the task. I'm approving your solution.
While your code works perfectly, there are a couple of opportunities to improve its performance, which are great learning points for building production-quality data structures:
put()method: You can make this method more efficient by avoiding the call togetNode(key). Since you already calculate theposition, you can directly access the head of the bucket withthis.elements[position]and traverse the list just once to find, update, or add a node.resize()method: A significant optimization here is to re-link the existing nodes to the new array instead of creating newNodeobjects for every element. This avoids unnecessary memory allocation and is much faster, especially for large maps.
These are suggestions for improvement, not required changes. You've done a great job fixing the previous issues. Keep up the fantastic progress!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| public void put(K key, V value) { | ||
| int hash = this.hash(key); | ||
| int position = hash % this.capacity; | ||
| Node<K, V> currentNode = this.getNode(key); |
There was a problem hiding this comment.
In the put method, you calculate the hash and position on lines 17 and 18. However, the getNode(key) call on this line recalculates the same values internally.
To avoid this redundant work, you can directly access the head of the bucket using the position you already calculated, like this:
Node<K, V> currentNode = this.elements[position];
| while (node != null) { | ||
| int newPosition = node.hash % newCap; | ||
| Node<K, V> newNode = new Node<>(node.hash, node.key, node.value, null); | ||
|
|
||
| if (newElements[newPosition] == null) { | ||
| newElements[newPosition] = newNode; | ||
| } else { | ||
| Node<K, V> currentNode = newElements[newPosition]; | ||
| Node<K, V> lastNode = null; | ||
|
|
||
| while (currentNode != null) { | ||
| lastNode = currentNode; | ||
| currentNode = currentNode.next; | ||
| } | ||
|
|
||
| lastNode.next = newNode; | ||
| } | ||
|
|
||
| node = node.next; | ||
| } |
There was a problem hiding this comment.
This implementation of resize correctly rehashes all the elements, but it's inefficient for two main reasons:
- Object Creation: A new
Nodeis created for every element in the map (line 78). It's more memory-efficient to move the existing nodes. - List Traversal: To add a node to a new bucket, you traverse the entire linked list in that bucket to find the end (lines 83-89). This can be very slow if collision chains get long.
A much more efficient approach is to prepend the old nodes to the new buckets. This avoids both creating new objects and traversing lists.
Consider this logic inside your while (node != null) loop:
Node<K, V> nextNode = node.next; // Save the next node from the old chain
int newPosition = node.hash % newCap;
node.next = newElements[newPosition]; // Link the current node to the start of the new chain
newElements[newPosition] = node; // The current node becomes the new head
node = nextNode; // Move to the next node in the old chain
No description provided.