Solved task with my own HashMap#1660
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because several checklist requirements are still violated and those are treated as mandatory, not optional style preferences. In MyHashMap.java, the helper remove(K key) is declared public (lines 70–89) even though it is an internal utility not required by the task API, which breaks encapsulation and violates the “utility methods should not be public” rule [CHECKLIST ITEM #6]; change it to private or remove it if unused. Also in MyHashMap, the private method resize() (lines 57–68) appears before the public methods, which goes against the required ordering [CHECKLIST ITEM #5]; move all private methods below the public ones. The resize multiplier 2 in tablica = new Node[oldTablica.length * 2]; (around line 59) must be extracted into a named constant such as private static final int RESIZE_MULTIPLIER = 2; to avoid magic numbers [CHECKLIST ITEM #2], and the generic array creation new Node[...] at lines 12 and 59 should be written with an explicit generic cast like (Node<K,V>[]) new Node[DEFAULT_INITIAL_CAPACITY]; to avoid raw-type/unchecked warnings [CHECKLIST ITEM #8]. You also need to rename non‑descriptive/non‑English identifiers (tablica, result, currentnode, oldTablica) to clear English names like table, bucketIndex, currentNode, oldTable for readability [CHECKLIST ITEM #9]. In Node.java, remove the redundant empty lines after the package statement and before the constructor [CHECKLIST ITEM #1], and add explicit access modifiers to the fields key, value, and next (for example: private final K key; private V value; private Node<K,V> next;) to restore encapsulation [CHECKLIST ITEM #7]. Once you address these points, your implementation will both meet the functional requirements and align with the checklist, and the core structure you already wrote (put/getValue/getSize, resizing, load factor) will be in very good shape.
✨ 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>[] oldTablica = tablica; | ||
| tablica = new Node[oldTablica.length * 2]; | ||
| size = 0; | ||
|
|
||
| for (Node<K, V> node : oldTablica) { | ||
| while (node != null) { | ||
| put(node.key, node.value); | ||
| node = node.next; | ||
| } | ||
| } | ||
| } |
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 method resize() (lines 57–68) appears before the public method remove(...) (lines 70–89). Move private methods and private classes to after all public methods to satisfy the checklist.
| public V remove(K key) { | ||
| int result = getBucketIndex(key); | ||
| Node<K, V> current = tablica[result]; | ||
| Node<K, V> prev = null; | ||
|
|
||
| while (current != null) { | ||
| if (Objects.equals(current.key, key)) { | ||
| if (prev == null) { | ||
| tablica[result] = current.next; | ||
| } else { | ||
| prev.next = current.next; | ||
| } | ||
| size--; | ||
| return current.value; // Dodano, żeby wyjść z metody z sukcesem | ||
| } | ||
| prev = current; | ||
| current = current.next; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This violates checklist item #6: "If a method has only a utility purpose and is used only inside the same class, it should not be public." The method remove(K key) is declared public (lines 70–89) but appears to be an internal utility (not required by the task public API). Change its visibility to private (or remove it if not needed).
|
|
||
| private void resize() { | ||
| Node<K, V>[] oldTablica = tablica; | ||
| tablica = new Node[oldTablica.length * 2]; |
There was a problem hiding this comment.
This violates checklist item #2: "Any magic numbers should be constants" — the resize multiplier 2 used in tablica = new Node[oldTablica.length * 2]; (line 59) is a hardcoded magic number. Extract it to a named constant (for example private static final int RESIZE_MULTIPLIER = 2).
| tablica = new Node[DEFAULT_INITIAL_CAPACITY]; | ||
| } | ||
|
|
||
| @Override | ||
| public void put(K key, V value) { | ||
| if (size >= tablica.length * DEFAULT_LOAD_FACTOR) { | ||
| resize(); | ||
| } | ||
|
|
||
| int result = getBucketIndex(key); | ||
| if (tablica[result] == null) { | ||
| tablica[result] = new Node<>(key, value, null); | ||
| size++; | ||
| } else { | ||
| Node<K, V> currentnode = tablica[result]; | ||
| while (currentnode != null) { | ||
| if (Objects.equals(currentnode.key, key)) { | ||
| currentnode.value = value; | ||
| return; | ||
| } | ||
| currentnode = currentnode.next; | ||
| } | ||
| tablica[result] = new Node<>(key, value, tablica[result]); | ||
| size++; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public V getValue(K key) { | ||
| int currentNodeIndex = getBucketIndex(key); | ||
| Node<K, V> current = tablica[currentNodeIndex]; | ||
| while (current != null) { | ||
| if (Objects.equals(current.key, key)) { | ||
| return current.value; | ||
| } | ||
| current = current.next; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public int getSize() { | ||
| return 0; | ||
| return size; | ||
| } | ||
|
|
||
| private void resize() { | ||
| Node<K, V>[] oldTablica = tablica; | ||
| tablica = new Node[oldTablica.length * 2]; |
There was a problem hiding this comment.
This violates checklist item #8: "Diamond operator
Be attentive with the diamond operator in java.
What is the difference between: new Node<>(...) and new Node<K,V>(...)and new Node(...)." You create generic arrays with raw-type new Node[...] at lines 12 and 59 which produces unchecked/generic-array warnings. Use an explicit cast such as tablica = (Node<K,V>[]) new Node[DEFAULT_INITIAL_CAPACITY]; and similarly in resize() to avoid warnings and make the intent explicit.
| private Node<K, V>[] tablica; | ||
| private int size; | ||
|
|
||
| public MyHashMap() { | ||
| tablica = new Node[DEFAULT_INITIAL_CAPACITY]; | ||
| } | ||
|
|
||
| @Override | ||
| public void put(K key, V value) { | ||
| if (size >= tablica.length * DEFAULT_LOAD_FACTOR) { | ||
| resize(); | ||
| } | ||
|
|
||
| int result = getBucketIndex(key); | ||
| if (tablica[result] == null) { | ||
| tablica[result] = new Node<>(key, value, null); | ||
| size++; | ||
| } else { | ||
| Node<K, V> currentnode = tablica[result]; | ||
| while (currentnode != null) { | ||
| if (Objects.equals(currentnode.key, key)) { | ||
| currentnode.value = value; | ||
| return; | ||
| } | ||
| currentnode = currentnode.next; | ||
| } | ||
| tablica[result] = new Node<>(key, value, tablica[result]); | ||
| size++; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public V getValue(K key) { | ||
| int currentNodeIndex = getBucketIndex(key); | ||
| Node<K, V> current = tablica[currentNodeIndex]; | ||
| while (current != null) { | ||
| if (Objects.equals(current.key, key)) { | ||
| return current.value; | ||
| } | ||
| current = current.next; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public int getSize() { | ||
| return 0; | ||
| return size; | ||
| } | ||
|
|
||
| private void resize() { | ||
| Node<K, V>[] oldTablica = tablica; |
There was a problem hiding this comment.
This violates checklist item #9: "Create informative variable and method names." Several identifiers are non-descriptive or non-English: field tablica (line 8), local variable result used as bucket index (line 21), currentnode (line 26), and oldTablica (line 58). Rename to descriptive English names, e.g. table or buckets, bucketIndex, currentNode, and oldTable to improve readability.
| @@ -0,0 +1,13 @@ | |||
| package core.basesyntax; | |||
|
|
|||
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's an empty line between the package declaration and the public class declaration — remove it.
| final K key; | ||
| V value; | ||
| Node <K,V> next; | ||
|
|
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's an empty line before the constructor (line 7) — remove redundant empty lines before method/class implementations.
| final K key; | ||
| V value; | ||
| Node <K,V> next; |
There was a problem hiding this comment.
This violates checklist item #7: "Remember about access modifiers. All fields and methods should have access modifiers. Remember about the encapsulation principle." The fields key, value, and next (lines 4–6) have no access modifiers. Make them explicit (for example: private final K key; private V value; private Node<K,V> next;).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still some checklist violations that are considered core requirements for this task, not just style preferences. In Node.java, you need to remove the redundant empty line after the package declaration, add an explicit access modifier to the constructor (Node(K key, V value, Node<K, V> next)), and remove all getters/setters so the map works with Node fields directly, as required by [CHECKLIST ITEM #1], [CHECKLIST ITEM #3], and [CHECKLIST ITEM #7]. In MyHashMap.java, change the generic array creation to use a properly cast generic array, for example in the constructor: tablica = (Node<K,V>[]) new Node[DEFAULT_INITIAL_CAPACITY];, and similarly in resize() (this is required by [CHECKLIST ITEM #8]). Also in resize(), extract the literal 2 used for resizing into a constant like private static final int RESIZE_MULTIPLIER = 2; and use oldTable.length * RESIZE_MULTIPLIER instead of the magic number [CHECKLIST ITEM #2]. Rename non‑English/uninformative identifiers tablica and oldTablica to something like table and oldTable to satisfy the naming requirements [CHECKLIST ITEM #9]. Finally, remove the extra blank lines before the MyHashMap constructor and the private methods resize() and getBucketIndex() so that method bodies don’t start with empty lines [CHECKLIST ITEM #1].
Once you fix these items, your implementation will better follow the project’s standards; the core logic is on the right track, so focus now on these checklist and naming adjustments to finalize a clean solution.
✨ 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
| @@ -0,0 +1,33 @@ | |||
| package core.basesyntax; | |||
|
|
|||
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 empty line immediately after the package statement. Remove the blank line so the class declaration follows the package line directly.
| private V value; | ||
| private Node<K, V> next; | ||
|
|
||
| Node(K key, V value, Node<K, V> next) { |
There was a problem hiding this comment.
This violates checklist item #7: "Remember about access modifiers. All fields and methods should have access modifiers. Remember about the encapsulation principle." — the constructor has no explicit access modifier (Node(K key, V value, Node<K, V> next)). Make the constructor's visibility explicit (for example public Node(...) or another appropriate modifier consistent with intended encapsulation).
| public K getKey() { | ||
| return key; | ||
| } | ||
|
|
||
| public V getValue() { | ||
| return value; | ||
| } | ||
|
|
||
| public void setValue(V value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| public Node<K, V> getNext() { | ||
| return next; | ||
| } | ||
|
|
||
| public 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" — Node exposes getKey(), getValue(), setValue(), getNext(), setNext(). The Node class is intended for internal use; remove these getters/setters and let the map implementation access Node fields directly (or make Node an inner class) to follow the checklist.
| private int size; | ||
|
|
||
| public MyHashMap() { | ||
| tablica = new Node[DEFAULT_INITIAL_CAPACITY]; |
There was a problem hiding this comment.
This violates checklist item #8: "Be attentive with the diamond operator in java. What is the difference between: new Node<>(...) and new Node<K,V>(...)and new Node(...)." The generic array is created with a raw-type new Node[...] here — use an explicit generic cast to avoid unchecked warnings, for example: (Node<K,V>[]) new Node[DEFAULT_INITIAL_CAPACITY];.
|
|
||
| private void resize() { | ||
| Node<K, V>[] oldTablica = tablica; | ||
| tablica = new Node[oldTablica.length * 2]; |
There was a problem hiding this comment.
This violates checklist item #2: "Any magic numbers should be constants" — the resize multiplier 2 is used directly here. Extract it to a named constant (for example private static final int RESIZE_MULTIPLIER = 2) and use it instead of the literal. Also avoid raw generic array creation here (see checklist item #8): use an explicit cast (Node<K,V>[]) new Node[oldTable.length * RESIZE_MULTIPLIER];.
| public class MyHashMap<K, V> implements MyMap<K, V> { | ||
| private static final int DEFAULT_INITIAL_CAPACITY = 16; | ||
| private static final float DEFAULT_LOAD_FACTOR = 0.75f; | ||
| private Node<K, V>[] tablica; |
There was a problem hiding this comment.
This violates checklist item #9: "Create informative variable and method names. Do not use abstract words like string or array as variable name..." — the field name tablica is non‑English/uninformative. Rename it to table, buckets or similar.
| } | ||
|
|
||
| private void resize() { | ||
| Node<K, V>[] oldTablica = tablica; |
There was a problem hiding this comment.
This violates checklist item #9: "Create informative variable and method names..." — the local variable oldTablica is non‑English/uninformative. Rename it to oldTable or oldBuckets.
| private Node<K, V>[] tablica; | ||
| private int size; | ||
|
|
||
| public MyHashMap() { |
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 :)" — the constructor public MyHashMap() starts after a redundant empty line. Remove the empty line so the method begins immediately.
| return size; | ||
| } | ||
|
|
||
| private void resize() { |
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 :)" — the private method resize() begins after a redundant empty line. Remove the extra blank line before the method declaration.
| } | ||
| } | ||
|
|
||
| private int getBucketIndex(K key) { |
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 :)" — the private method getBucketIndex() begins after a redundant empty line. Remove the blank line before the method declaration.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can continue progressing while you learn and refine your style and edge-case handling. You implemented the core MyHashMap functionality well: put(K,V), getValue(K), resizing logic, constants for initial capacity/load factor, and overwriting existing keys are all handled correctly and clearly. There are a few style issues related to [CHECKLIST ITEM #1]: remove the extra blank line after the imports before the class declaration, the blank line before the constructor body, and the blank lines before the resize(), getBucketIndex(), and Node inner class so that no class or method starts with an empty line. More importantly, your getBucketIndex currently uses Math.abs(key.hashCode() % table.length), which can still be negative for Integer.MIN_VALUE; instead, use a non-negative transform like int idx = (key.hashCode() & 0x7fffffff) % table.length; to guarantee a valid array index. Once you fix the formatting and make the bucket index strictly non-negative, your implementation will be both clean and robust, and overall you’re very close to a polished, production-quality solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| package core.basesyntax; | ||
|
|
||
| import java.util.Objects; | ||
|
|
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 an unnecessary blank line here (separating imports/class). Remove it so the class declaration starts immediately after imports.
| private static final int RESIZE_MULTIPLIER = 2; | ||
| private Node<K, V>[] table; | ||
| private int size; | ||
|
|
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 empty line immediately before the constructor. Remove it so the constructor body doesn't start after a blank line.
| return 0; | ||
| return size; | ||
| } | ||
|
|
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 :)" — remove the blank line immediately before the resize() method so the method starts without a leading empty line.
| } | ||
| } | ||
| } | ||
|
|
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 :)" — remove the blank line immediately before the getBucketIndex() method.
| private int getBucketIndex(K key) { | ||
| return (key == null) ? 0 : Math.abs(key.hashCode() % table.length); | ||
| } | ||
|
|
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 :)" — remove the blank line immediately before the Node inner class declaration.
| } | ||
|
|
||
| private int getBucketIndex(K key) { | ||
| return (key == null) ? 0 : Math.abs(key.hashCode() % table.length); |
There was a problem hiding this comment.
Potential correctness bug: Math.abs(key.hashCode() % table.length) can still produce a negative index when key.hashCode() % table.length == Integer.MIN_VALUE (Math.abs(Integer.MIN_VALUE) is negative). Use a non-negative transform before modulo, for example: int hash = key.hashCode() & 0x7fffffff; return hash % table.length; or int idx = (key.hashCode() & 0x7fffffff) % table.length; to guarantee a non-negative index.
No description provided.