solution for jv-hash-map#1694
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because, while your MyHashMap implementation appears functionally correct, there are several explicit checklist and style requirements that are not yet met. The inner Node class and its constructor currently lack explicit access modifiers (around the bottom of MyHashMap.java); per the checklist, make the class something like private static class Node<K, V> and give its constructor an explicit modifier (e.g., private). Private helper methods (calculateThreshold, resize, index) appear before the public API methods (put, getValue, getSize); you should move these helpers after the public methods to match the required method-ordering convention. In resize (around the place where you double capacity with currentCapacity * 2) and in your null-key hashing logic where you use 0 as a special hash, replace these magic numbers with well‑named constants (e.g., RESIZE_MULTIPLIER, NULL_KEY_HASH) and reuse them consistently. Your table/array creation for Node currently uses a raw generic array without the same cast form you use elsewhere; unify this by always creating the array as (Node<K, V>[]) new Node[currentCapacity] (optionally with a localized @SuppressWarnings("unchecked")) so you avoid raw types and inconsistent patterns. Finally, remove redundant empty lines at the very start of the file and directly before the calculateThreshold, resize, index methods, and before the inner Node class declaration to comply with the "no leading empty line" checklist rule and keep formatting clean. Once you address these issues, your solution will both meet the functional requirements and align with the style and checklist expectations.
✨ 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
| package core.basesyntax; | ||
|
|
||
| import java.util.Objects; | ||
|
|
There was a problem hiding this comment.
This begins the file with an unnecessary empty line before the class declaration. Fix: remove the empty line. (Checklist: "Don't begin class or method implementation with empty line.")
| private int currentThreshold = initialThreshold; | ||
| private final double defaultLoadFactor = 0.75; | ||
| private int size = 0; | ||
|
|
There was a problem hiding this comment.
There is an empty line immediately before the calculateThreshold method. Remove redundant blank line at the start of method implementation. (Checklist: "Don't begin class or method implementation with empty line.")
| private int calculateThreshold(int currentCapacity, double defaultLoadFactor) { | ||
| return (int) (defaultLoadFactor * currentCapacity); | ||
| } | ||
|
|
There was a problem hiding this comment.
There is an empty line immediately before the resize method. Remove redundant blank line at the start of method implementation. (Checklist: "Don't begin class or method implementation with empty line.")
| currentCapacity = newCapacity; | ||
| currentThreshold = calculateThreshold(currentCapacity, defaultLoadFactor); | ||
| } | ||
|
|
There was a problem hiding this comment.
There is an empty line immediately before the index method. Remove redundant blank line at the start of method implementation. (Checklist: "Don't begin class or method implementation with empty line.")
| return 0; | ||
| return size; | ||
| } | ||
|
|
There was a problem hiding this comment.
There is an empty line immediately before the inner Node class declaration. Remove redundant blank line at the start of class implementation. (Checklist: "Don't begin class or method implementation with empty line.")
| currentThreshold = calculateThreshold(currentCapacity, defaultLoadFactor); | ||
| table = (Node<K, V>[]) new Node[currentCapacity]; | ||
| } | ||
| int hash = (key == null) ? 0 : key.hashCode(); |
There was a problem hiding this comment.
You use the literal 0 as the default hash for null keys ((key == null) ? 0 : key.hashCode()). Consider extracting common magic values like this to constants with informative names (e.g., NULL_KEY_HASH). (Checklist: "Any magic numbers should be constants")
| if (table == null || table.length == 0) { | ||
| return null; | ||
| } else { | ||
| int hash = (key == null) ? 0 : key.hashCode(); |
There was a problem hiding this comment.
Same as above: use a named constant for the null-key hash in getValue instead of the literal 0. (Checklist: "Any magic numbers should be constants")
| private int calculateThreshold(int currentCapacity, double defaultLoadFactor) { | ||
| return (int) (defaultLoadFactor * currentCapacity); | ||
| } | ||
|
|
||
| private void resize() { |
There was a problem hiding this comment.
Private helper methods are declared before the public API methods. Per checklist, private methods (and classes) should be placed after public ones. Move calculateThreshold, resize, and index after the public methods. (Checklist: "Private methods and classes should be after public ones in your class.")
| return size; | ||
| } | ||
|
|
||
| static class Node<K, V> { |
There was a problem hiding this comment.
The inner Node class lacks an explicit access modifier. Add an access modifier (e.g., private static class Node<K, V>) to follow encapsulation and checklist rules. (Checklist: "Remember about access modifiers.")
| private V value; | ||
| private Node<K, V> next; | ||
|
|
||
| Node(int hash, K key, V value) { |
There was a problem hiding this comment.
The Node constructor has no explicit access modifier (package-private). All fields and methods should have access modifiers; make the constructor private if Node remains private. (Checklist: "All fields and methods should have access modifiers.")
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a few checklist requirements on constants, generics, and access modifiers are still not met, even though the core MyHashMap functionality works correctly.
Critical issues to fix:
- In
MyHashMap.java, the initial capacity, threshold, and load factor are currently hardcoded as instance fields (capacity = 16,initialThreshold = 12,defaultLoadFactor = 0.75near the top of the class); these should be promoted tostatic finalconstants with clear names likeDEFAULT_CAPACITY,DEFAULT_LOAD_FACTOR, and either a computed threshold or a named constant, to satisfy the “no magic numbers” checklist requirement. - The literal
0is used directly when handling the null key hash (for example in the branch that handleskey == null); introduce a constant such asprivate static final int NULL_KEY_HASH = 0and use it wherever you currently use0for the null-key hash. - When you initialize the
Nodetable (the first allocation of the backing array), you currently usenew Node[currentCapacity]without a generic cast; align this with your other allocations by using(Node<K, V>[]) new Node[currentCapacity]and, if needed, add a local@SuppressWarnings("unchecked")above that line. - The inner
Nodeconstructor is missing an explicit access modifier (it is package‑private); update the constructor signature to include an explicit modifier, for exampleprivate Node(int hash, K key, V value, Node<K, V> next), to meet the “all fields and methods should have access modifiers” rule.
Once you address these points, your solution will both satisfy the formal checklist and keep the implementation clean and maintainable; the functional parts of your map (put/get/resize) are already in good shape, so you’re very close to a fully polished submission.
✨ 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> { | ||
|
|
||
| public static final int RESIZE_MULTIPLIER = 2; | ||
| private final int initialCapacity = 16; |
There was a problem hiding this comment.
This violates checklist item: "Any magic numbers should be constants". The capacity value is declared as an instance final field. Make it a named constant (static final) e.g. private static final int DEFAULT_CAPACITY = 16 and reuse that constant.
|
|
||
| public static final int RESIZE_MULTIPLIER = 2; | ||
| private final int initialCapacity = 16; | ||
| private final int initialThreshold = 12; |
There was a problem hiding this comment.
This violates checklist item: "Any magic numbers should be constants". initialThreshold is a hardcoded number (12). Either compute it from capacity and load factor or make it a named constant (static final) with a descriptive name.
| private int currentCapacity = initialCapacity; | ||
| private Node<K, V>[] table = new Node[currentCapacity]; | ||
| private int currentThreshold = initialThreshold; | ||
| private final double defaultLoadFactor = 0.75; |
There was a problem hiding this comment.
This violates checklist item: "Any magic numbers should be constants". defaultLoadFactor should be a constant (static final) and named accordingly (e.g. DEFAULT_LOAD_FACTOR).
| private final int initialCapacity = 16; | ||
| private final int initialThreshold = 12; | ||
| private int currentCapacity = initialCapacity; | ||
| private Node<K, V>[] table = new Node[currentCapacity]; |
There was a problem hiding this comment.
Per the previous review guidance: "Your table/array creation for Node currently uses a raw generic array without the same cast form you use elsewhere; unify this by always creating the array as (Node<K, V>[]) new Node[currentCapacity] ...". Here you use new Node[currentCapacity] without the generic cast — make this consistent and consider a localized @SuppressWarnings("unchecked").
| currentThreshold = calculateThreshold(currentCapacity, defaultLoadFactor); | ||
| table = (Node<K, V>[]) new Node[currentCapacity]; | ||
| } | ||
| int hash = (key == null) ? 0 : key.hashCode(); |
There was a problem hiding this comment.
This violates the previous review suggestion: "Replace specific magic numbers with named constants and reuse them consistently (e.g., RESIZE_MULTIPLIER, NULL_KEY_HASH)". The literal 0 is used here for null-key hashing. Introduce a named constant (e.g. private static final int NULL_KEY_HASH = 0) and use it instead of 0.
| if (table == null || table.length == 0) { | ||
| return null; | ||
| } else { | ||
| int hash = (key == null) ? 0 : key.hashCode(); |
There was a problem hiding this comment.
Same issue as above: the literal 0 is used for null-key hashing. Replace with a named constant such as NULL_KEY_HASH to satisfy: "Replace specific magic numbers with named constants and reuse them consistently (e.g., RESIZE_MULTIPLIER, NULL_KEY_HASH)".
| private V value; | ||
| private Node<K, V> next; | ||
|
|
||
| Node(int hash, K key, V value) { |
There was a problem hiding this comment.
This violates checklist item: "Give the inner Node class and its constructor explicit access modifiers... give its constructor an explicit modifier (e.g., private)." The inner class has an explicit modifier but the constructor has package-private access. Make the constructor explicit (for example private Node(int hash, K key, V value)).
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 keep progressing, but there are a few important style and checklist issues in MyHashMap that you should fix next.
Your core logic for put, getValue, getSize, and resizing is solid, including handling of null keys, which is a strong foundation.
The most critical thing to fix is the constant setup near the top of the class: the public static final int DEFAULT_LOAD_FACTOR = 12;, the instance private final double defaultLoadFactor = 0.75;, and private int currentThreshold = DEFAULT_LOAD_FACTOR; are confusing and conflict with the requirement that load factor and threshold be clear static final constants; instead, define something like private static final int DEFAULT_CAPACITY = 16; and private static final double DEFAULT_LOAD_FACTOR = 0.75;, and compute the threshold from these (e.g., currentThreshold = (int) (currentCapacity * DEFAULT_LOAD_FACTOR);).
Another critical issue is the backing array declaration private Node<K, V>[] table = new Node[currentCapacity];, which uses a raw array type; you should switch this to a generic array creation with a cast, for example @SuppressWarnings("unchecked") private Node<K, V>[] table = (Node<K, V>[]) new Node[DEFAULT_CAPACITY]; and apply the same pattern wherever you reallocate the table during resize.
As minor improvements, keep all constants as private static final with ALL_CAPS names (e.g., remove the instance defaultLoadFactor field, use a single DEFAULT_LOAD_FACTOR constant, and avoid reusing NULL_KEY_HASH for things like length checks that are not related to null-key hashing).
Once you clean up the constants, threshold calculation, and generic array initialization, your implementation will not only work correctly but also align well with the style and checklist expectations—nice work getting the main behavior right, and treat these fixes as polishing your solution to a professional level.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| public static final int RESIZE_MULTIPLIER = 2; | ||
| public static final int DEFAULT_CAPACITY = 16; | ||
| public static final int DEFAULT_LOAD_FACTOR = 12; |
There was a problem hiding this comment.
This violates the requirement to replace hardcoded instance fields with clear static final constants. The declaration public static final int DEFAULT_LOAD_FACTOR = 12; (an int named "DEFAULT_LOAD_FACTOR") is misleading and incorrect. From the task guidance: "HIGH: Replace hardcoded instance fields for initial capacity, threshold, and load factor with static final constants with clear names, instead of values like capacity = 16, initialThreshold = 12, and defaultLoadFactor = 0.75: “these should be promoted to static final constants … to satisfy the ‘no magic numbers’ checklist requirement.”" Consider making a private static final double DEFAULT_LOAD_FACTOR = 0.75; (or similar clearly named constants) and compute the initial threshold from capacity and load factor rather than hardcoding an integer 12.
| private int currentCapacity = DEFAULT_CAPACITY; | ||
| private Node<K, V>[] table = new Node[currentCapacity]; | ||
| private int currentThreshold = DEFAULT_LOAD_FACTOR; | ||
| private final double defaultLoadFactor = 0.75; |
There was a problem hiding this comment.
The instance field private final double defaultLoadFactor = 0.75; is not declared as a constant. This conflicts with the checklist: "Any magic numbers should be constants" and the prior guidance to promote load factor to a static final constant. Make the load factor a static final constant (e.g. private static final double DEFAULT_LOAD_FACTOR = 0.75;) and remove the duplicate/misnamed int constant.
| public static final int DEFAULT_LOAD_FACTOR = 12; | ||
| private static final int NULL_KEY_HASH = 0; | ||
| private int currentCapacity = DEFAULT_CAPACITY; | ||
| private Node<K, V>[] table = new Node[currentCapacity]; |
There was a problem hiding this comment.
This line initializes the backing table using a raw type: private Node<K, V>[] table = new Node[currentCapacity];. The previous guidance requires a properly parameterized array creation and a cast. From the requirements: "When initializing the Node table, use a properly parameterized array creation with a cast: ... use (Node<K, V>[]) new Node[currentCapacity] and, if needed, add a local @SuppressWarnings("unchecked") above that line." Change this initialization to a parameterized creation with the appropriate cast and add @SuppressWarnings("unchecked") as needed.
| private static final int NULL_KEY_HASH = 0; | ||
| private int currentCapacity = DEFAULT_CAPACITY; | ||
| private Node<K, V>[] table = new Node[currentCapacity]; | ||
| private int currentThreshold = DEFAULT_LOAD_FACTOR; |
There was a problem hiding this comment.
private int currentThreshold = DEFAULT_LOAD_FACTOR; uses the misleading int constant DEFAULT_LOAD_FACTOR (12). This ties back to the checklist "Any magic numbers should be constants" and the earlier HIGH guidance to promote threshold/load-factor handling to clear constants/computed values. Instead of assigning threshold to that int, compute it from capacity and the (double) load factor, e.g. currentThreshold = calculateThreshold(DEFAULT_CAPACITY, DEFAULT_LOAD_FACTOR); after you define DEFAULT_LOAD_FACTOR as a double static final.
No description provided.