Task from README was done by melnyk317 · Pull Request #1897 · mate-academy/jv-storage

melnyk317 · 2026-03-26T13:19:57Z

No description provided.

mateacademy-ai-mentor

Review Summary

This submission requires changes because of a critical bug that breaks the core functionality. Both the put and get methods will crash with a NullPointerException because your loops iterate over the entire storage array, including the empty (null) slots. The loops should only iterate up to the number of elements that have actually been added.

The root cause is the class design, where StorageImpl acts as both the container and the data entry. A simpler approach that would resolve the bug is to use two parallel arrays—one for keys and one for values.

Additionally, please address these checklist violations:

The array size 10 is a magic number and should be declared as a private static final constant. [CHECKLIST ITEM #3]
Instance variables like storage and count should be initialized within the constructor. [CHECKLIST ITEM #2]

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-03-26T13:22:26Z

+    private K key;
+    private V value;


This class has fields key and value, but it also contains an array of itself (StorageImpl[] storage). This design is confusing because the class is trying to be both a single key-value entry and a container for many entries. A cleaner approach is to have the StorageImpl class act only as a container. The key-value pairs can be stored in two separate arrays, for example private K[] keys; and private V[] values;.

mateacademy-ai-mentor · 2026-03-26T13:22:26Z

+    public StorageImpl() {
+    }
+
+    StorageImpl[] storage = new StorageImpl[10];


This field should be initialized in the constructor, as per checklist item #2. Also, the number 10 is a magic number and should be declared as a private static final constant, as required by checklist item #3.

mateacademy-ai-mentor · 2026-03-26T13:22:27Z

+    }
+
+    StorageImpl[] storage = new StorageImpl[10];
+    int count = 0;


This non-constant field should be initialized in the constructor, as per checklist item #2.

mateacademy-ai-mentor · 2026-03-26T13:22:27Z

+
    @Override
    public void put(K key, V value) {
+        for (StorageImpl st : storage) {


This loop will iterate over the entire storage array, including the null elements at the end. When st is null, the expression st.key will cause a NullPointerException. The loop should only iterate over the elements that have already been added (i.e., from index 0 up to count).

mateacademy-ai-mentor · 2026-03-26T13:22:27Z

+        for (StorageImpl st : storage) {
+            if (st.key.equals(key)) {
+                st.value = value;
+                return;
+            }
+        }
+        storage[count] = new StorageImpl<K, V>(key, value);
+        count++;
    }

    @Override
    public V get(K key) {
+        for (StorageImpl st : storage) {
+            if (st.key.equals(key)) {
+                return (V) st.value;
+            }
+        }


The logic for finding an existing key is duplicated in both the put and get methods. This violates checklist item #5. This logic should be extracted into a separate private helper method to avoid repetition (DRY principle). For example, a method like private int findIndexOfKey(K key) could return the array index of a given key.

mateacademy-ai-mentor · 2026-03-26T13:22:27Z


    @Override
    public V get(K key) {
+        for (StorageImpl st : storage) {


This loop has the same issue as the one in the put method. It will throw a NullPointerException when it tries to access a property of a null element in the storage array. You should only iterate over the elements that have been added.

mateacademy-ai-mentor

Review Summary

Great job on the progress so far, especially with fixing the previous bug! However, a few changes are needed before this can be approved.

There's a potential NullPointerException in your put and get methods. If a null key is stored, the line keys[i].equals(key) can crash if keys[i] is null. You need to handle comparisons safely when either the existing key or the new key might be null.

Additionally, your solution violates a core requirement: all non-constant fields must be initialized in the constructor [CHECKLIST ITEM #2]. Please move the initialization for keys, values, and count from their declarations into the StorageImpl() constructor.

Finally, there are some unused fields (key, value) and a related constructor that should be removed to clean up the code. Please address these points and resubmit!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-03-26T15:23:34Z

+    private K key;
+    private V value;


These fields are declared but never used within the class. Unused fields can make the code harder to understand and should be removed.

mateacademy-ai-mentor · 2026-03-26T15:23:34Z

+    private Object[] keys = new Object[MAX_ARRAY_SIZE];
+    private Object[] values = new Object[MAX_ARRAY_SIZE];
+    int count = 0;


This violates checklist item #2: 'All non-constant fields should be initialized in the constructor'. The initialization for keys, values, and count should be moved to the StorageImpl() constructor.

mateacademy-ai-mentor · 2026-03-26T15:23:34Z

+    public StorageImpl(K key, V value) {
+        this.key = key;
+        this.value = value;
+    }


This constructor initializes the unused key and value fields. It should be removed along with them to clean up the code.

mateacademy-ai-mentor · 2026-03-26T15:23:34Z

+
+    private int findIndexOfKey(K key) {
+        for (int i = 0; i < count; i++) {
+            if (keys[i].equals(key)) {


This equality check can cause a NullPointerException. If a null key is stored (e.g., via put(null, value)), keys[i] can become null. Calling keys[i].equals(key) will then crash. You must handle comparisons involving potential null values safely.

mateacademy-ai-mentor

Review Summary

I am approving your solution as an educational override to help you move forward. You've done an excellent job fixing the critical NullPointerException and correctly initializing the fields in the constructor—the code is much safer now!

However, there are a couple of small things to keep in mind for the future:

The unused constructor public StorageImpl(K key, V value) should be removed to keep the code clean.
The build failed due to a Checkstyle error with the import order. You can usually fix this automatically in your IDE by using the 'Organize Imports' feature.

Great progress on this task. Keep up the excellent work!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2026-03-27T14:55:16Z

+    private Object[] values;
+    private int count;
+
+    public StorageImpl(K key, V value) {


The parameters key and value in this constructor are not used. Since this constructor provides the same functionality as the default constructor, it should be removed to avoid confusion and unnecessary code.

Task from README was done

344ad21

mateacademy-ai-mentor suggested changes Mar 26, 2026

View reviewed changes

darina_melnyk added 2 commits March 26, 2026 16:49

Fix review comments

5e72097

Fix review comments

85bfd7f

melnyk317 requested a review from mateacademy-ai-mentor March 26, 2026 15:21

mateacademy-ai-mentor suggested changes Mar 26, 2026

View reviewed changes

darina_melnyk added 3 commits March 26, 2026 18:35

Fix review comments

d1438c1

Fix review comments

12e8ba5

Fix review comments

abf0816

melnyk317 requested a review from mateacademy-ai-mentor March 27, 2026 14:53

mateacademy-ai-mentor approved these changes Mar 27, 2026

View reviewed changes

Fix review comments

94e79a1

Conversation

melnyk317 commented Mar 26, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants