Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is a real correctness risk in your index calculation and several explicit checklist requirements are not yet met. The index computation using Math.abs(hashCode) % table.length (both in getIndex and in resize) can still produce a negative value for Integer.MIN_VALUE, which may cause ArrayIndexOutOfBoundsException; please switch to a safe non‑negative form such as (hash & 0x7FFFFFFF) % length or Integer.remainderUnsigned(hash, length) and apply it consistently. The private static inner Node class is currently declared before the public methods (put, getValue, getSize), which violates the rule that public API should come first [CHECKLIST ITEM #5]; move the Node class (and other private helpers) below the public methods. Inside Node, the fields and constructor lack access modifiers (final K key; V value; Node<K,V> next; Node(K key, V value)), which breaks the “all fields and methods must have explicit access modifiers” rule [CHECKLIST ITEM #7]; mark them private (e.g., private final K key; private V value; private Node<K,V> next; and a private constructor). When creating the backing array and new arrays in resize, you are using raw new Node[...] which leads to unchecked conversion; instead, create them as Node<K,V>[] table = (Node<K,V>[]) new Node[DEFAULT_CAPACITY]; (and similarly for newTable) to avoid raw types, optionally with @SuppressWarnings("unchecked"). Finally, remove leading empty lines at the start of the class and method bodies to comply with the no-leading-empty-line rule [CHECKLIST ITEM #1]. Once you address these points, your overall logic (put/getValue/getSize, resizing, and chaining) should be in good shape for acceptance.
✨ 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
| threshold = (int) (DEFAULT_CAPACITY * LOAD_FACTOR); | ||
| } | ||
|
|
||
| private static class Node<K, V> { |
There was a problem hiding this comment.
This violates checklist item #5: "Private methods and classes should be after public ones in your class." The private static Node class is declared before the public API (put, getValue, getSize). Move the Node class below the public methods to satisfy the checklist and improve class structure.
| private final K key; | ||
| private V value; | ||
| private Node<K, V> next; | ||
|
|
||
| Node(K key, V value) { | ||
| this.key = key; | ||
| this.value = value; | ||
| } |
There was a problem hiding this comment.
This violates checklist item #7: "Remember about access modifiers. All fields and methods should have access modifiers." The inner Node fields and its constructor have no explicit access modifiers (final K key; V value; Node<K, V> next; and Node(K key, V value)). Make these private (for example: private final K key;, private V value;, private Node<K,V> next;) and the constructor private to preserve encapsulation.
| private int threshold; | ||
|
|
||
| public MyHashMap() { | ||
| table = new Node[DEFAULT_CAPACITY]; |
There was a problem hiding this comment.
This relates to checklist item #8 (diamond operator / generic usage): creating the table as new Node[DEFAULT_CAPACITY]; uses a raw type and causes unchecked-conversion warnings. Use an explicit cast when creating generic arrays: table = (Node<K,V>[]) new Node[DEFAULT_CAPACITY]; to make the conversion explicit and avoid raw-type usage.
|
|
||
| private void resize() { | ||
| int newCapacity = table.length * 2; | ||
| Node<K, V>[] newTable = new Node[newCapacity]; |
There was a problem hiding this comment.
Same issue as above for newTable: new Node[newCapacity] is a raw array creation. Use Node<K,V>[] newTable = (Node<K,V>[]) new Node[newCapacity]; to avoid raw-type/unchecked warnings and be consistent with generics.
| private int getIndex(K key) { | ||
| if (key == null) { | ||
| return 0; | ||
| } | ||
| return Math.abs(key.hashCode()) % table.length; |
There was a problem hiding this comment.
Correctness issue in index calculation: using Math.abs(key.hashCode()) % table.length can still produce a negative value when hashCode() returns Integer.MIN_VALUE, leading to a negative index and potential ArrayIndexOutOfBoundsException. Replace with a non-negative-safe computation such as (key.hashCode() & 0x7FFFFFFF) % table.length or Integer.remainderUnsigned(key.hashCode(), table.length) to guarantee a non-negative index. Apply this fix in getIndex.
| int newIndex = (node.key == null) | ||
| ? 0 | ||
| : Math.abs(node.key.hashCode()) % newCapacity; |
There was a problem hiding this comment.
Same Math.abs issue inside resize() when computing newIndex: Math.abs(node.key.hashCode()) % newCapacity can be negative for Integer.MIN_VALUE. Use the same non-negative-safe computation suggested above when rehashing nodes.
| public class MyHashMap<K, V> implements MyMap<K, V> { | ||
|
|
There was a problem hiding this comment.
This violates checklist item #1: "Don't begin class or method implementation with empty line." The class and some methods contain leading empty lines inside their bodies (for example immediately after the class declaration). Remove redundant empty lines at starts of class and method implementations.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because the required functionality of MyHashMap is correctly implemented (put, getValue, getSize), resizing works as expected, and the index computation safely avoids negative indices. The remaining issues are minor style/checklist items rather than functional blockers.
Critical checklist-related notes for you to fix in the next iteration:
- There are redundant blank lines in
MyHashMap.javathat violate the checklist rule [CHECKLIST ITEM #1]: for example, the blank line immediately before the constructor (around the first constructor declaration) and the blank line directly before theputmethod declaration. Remove those empty lines so that each constructor and method starts right after the previous declaration or closing brace, without a leading blank line.
As a small improvement, consider extracting 0x7FFFFFFF and the resize multiplier (e.g., 2) into named constants to avoid magic numbers and improve readability. Overall, the logic and structure look solid, and with these minor style adjustments you’ll be fully aligned with the checklist—nice work on the core implementation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| private Node<K, V>[] table; | ||
| private int size; | ||
| private int threshold; | ||
|
|
There was a problem hiding this comment.
This violates checklist item #1: "Don't begin class or method implementation with empty line. Remove all redundant empty lines, be careful :)" — there is a redundant blank line immediately before the constructor; remove it so the constructor starts directly after the previous declaration.
| threshold = (int) (DEFAULT_CAPACITY * LOAD_FACTOR); | ||
| size = 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
This violates checklist item #1: "Don't begin class or method implementation with empty line. Remove all redundant empty lines, be careful :)" — there is a redundant blank line just before the put method. Remove it so the method annotation and declaration are contiguous.
No description provided.