first commit#1703
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your HashMap implementation is functionally complete and correct - all core requirements (put, get, size, initial capacity, load factor, resize behavior, collision handling) are properly implemented. The code works as expected. However, two types of issues need addressing before final approval: 1. Access Modifier Violations [CHECKLIST ITEM #6]: checkThreshold() (called from put()) and resize() (called from checkThreshold()) are declared public but only used internally. Change both to private. 2. Checkstyle Violations (5 total): - Lines 6-7: Static constants should be declared before other static variables - Line 10: Remove one blank line before the constructor (keep exactly one) - Lines 41, 63: Break lines longer than 100 characters into multiple lines. These are formatting and encapsulation refinements rather than functional problems. Once you correct the private visibility on those two methods and fix the checkstyle formatting issues, your implementation will be fully compliant with all requirements.
✨ 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
| size = 0; | ||
| } | ||
|
|
||
| public void checkThreshold() { |
There was a problem hiding this comment.
This method is only called internally within the class (from put() method). According to checklist item #6, if a method has only a utility purpose and is used only inside the same class, it should not be public. Change to private.
| } | ||
| } | ||
|
|
||
| public void resize() { |
There was a problem hiding this comment.
This method is only called internally within the class (from checkThreshold() method). According to checklist item #6, if a method has only a utility purpose and is used only inside the same class, it should not be public. Change to private.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is working correctly with all core HashMap methods properly functional. The remaining issues are style and formatting violations (checkstyle errors and a magic number that should be a constant), not functional blockers. Magic number '2' in resize() should be replaced with a named constant like CAPACITY_MULTIPLIER = 2. Lines 41 and 63 exceed 100 characters and should be broken into multiple lines for readability. Checkstyle violations include static variables ordering and extra blank lines before constructor. These are all minor style issues that don't affect functionality. Fix them to pass the linting checks, but your core implementation is sound.
✨ 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]; |
There was a problem hiding this comment.
This violates checklist item #2: 'Any magic numbers should be constants'. The value 2 (for doubling capacity) should be declared as a named constant (e.g., CAPACITY_MULTIPLIER) for better code readability.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Educational Approval: I'm approving your solution so you can continue progressing while learning. Your core HashMap implementation is solid—put, getValue, getSize, resizing logic, and the parameterized Node<K, V> class are all correctly implemented. However, please address these style issues before your next submission: (1) Remove the empty lines before your constructor (lines 9-12 should have only one blank line), (2) Reorder your static variables to follow Java conventions—place DEFAULT_CAPACITY, DEFAULT_RESIZE, and LOAD_FACTOR at the very top of the class before instance variables table and size, (3) Add spaces around the = sign on line 7 (DEFAULT_CAPACITY = 16), and (4) Break lines 43 and 65 which exceed 100 characters. Keep up the good work on the logic—you're on the right track!
✨ 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; | ||
|
|
There was a problem hiding this comment.
This violates checklist item #1: 'Don't begin class or method implementation with empty line.' There should be no empty lines between the package declaration and class declaration.
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
There should be no empty lines before the class declaration. Remove lines 9, 10, 11.
slade13
left a comment
slade13
left a comment
There was a problem hiding this comment.
Good job! Please review my comments, add necessary updates, and additionally your code does not pass the build on GitHub:
https://github.com/mate-academy/jv-hash-map/actions/runs/26342604012/job/77547125209?pr=1703
| package core.basesyntax; | ||
|
|
||
| public class MyHashMap<K, V> implements MyMap<K, V> { |
There was a problem hiding this comment.
Add empty line after package declaration.
| private void checkThreshold() { | ||
| if (size >= table.length * LOAD_FACTOR) { | ||
| resize(); | ||
| } | ||
| } | ||
|
|
||
| private void resize() { | ||
| Node<K, V>[] oldTable = table; | ||
| table = new Node[oldTable.length * DEFAULT_RESIZE]; | ||
| size = 0; | ||
|
|
||
| for (Node<K, V> node : oldTable) { | ||
| while (node != null) { | ||
| put(node.key, node.value); | ||
| node = node.next; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
According to the checklist:
Private methods and classes should be after public ones in your class.
Currently, checkThreshold() and resize() are placed before the public methods put(), getValue(), and getSize().
| private static class Node<K, V> { | ||
| private final K key; | ||
| private V value; | ||
| private int hash; |
There was a problem hiding this comment.
The hash field in Node is redundant, in put(), you assign:
newNode.hash = hash;
but this field is not used anywhere later.
In Node, you currently have something like:
private int hash;
This field can be removed because it does not bring any value to the current implementation.
|
|
||
| public class MyHashMap<K, V> implements MyMap<K, V> { | ||
| private static final int DEFAULT_CAPACITY = 16; | ||
| private static final int DEFAULT_RESIZE = 2; |
There was a problem hiding this comment.
The constant name DEFAULT_RESIZE is not very clear
You currently have:
private static final int DEFAULT_RESIZE = 2;
This name is not very descriptive. The value does not represent a resize itself, but rather a multiplier used during resizing.
A better name would be:
| private static final int DEFAULT_RESIZE = 2; | |
| private static final int RESIZE_MULTIPLIER = 2; |
| } | ||
|
|
||
| private void checkThreshold() { | ||
| if (size >= table.length * LOAD_FACTOR) { |
There was a problem hiding this comment.
Resize is triggered after adding a new element, currently, the logic is:
size++;
checkThreshold();
This means that the map first adds the new element and only then checks whether resizing is needed.
This is not necessarily a critical bug, but the current condition:
if (size >= table.length * LOAD_FACTOR)
means that for the default capacity 16 and load factor 0.75, resizing happens immediately after adding the 12th element.
Usually, it is slightly clearer to resize only when the threshold is exceeded:
if (size > table.length * LOAD_FACTOR) {
resize();
}
or to check before adding the new element:
if (size + 1 > table.length * LOAD_FACTOR) {
resize();
}
The current solution can work, but this small change would make the behavior more natural.
3 participants
No description provided.