First solution by Viodon by Viodon · Pull Request #1914 · mate-academy/jv-storage

Viodon · 2026-05-10T18:03:41Z

No description provided.

mateacademy-ai-mentor

Review Summary

Your implementation has a critical bug in the put() method that completely breaks core functionality. The empty slot detection on line 14 checks keys[i] == null && values[i] == null, but after inserting a key, values[i] becomes non-null, so subsequent put() calls will never find empty slots—only one element can ever be stored. You need to change this condition to only check keys[i] == null. Additionally, there are checklist violations that must be fixed: fields keys and values should be initialized in the constructor, not inline at field declaration [CHECKLIST ITEM #2]. The put() method also contains duplicate code patterns where two identical values[i] = value; return; blocks appear for different conditions [CHECKLIST ITEM #5]. Finally, remove the empty line before the @Override annotation [CHECKLIST ITEM #1]. The reference equality check keys[i] == key on line 18 is redundant since keys[i].equals(key) already handles all cases including null keys. Once you fix the critical bug, the storage should be able to hold up to 10 elements as required.

✨ 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-05-10T18:05:37Z

 import core.basesyntax.Storage;

 public class StorageImpl<K, V> implements Storage<K, V> {
+


Violates checklist item #1: Remove all redundant empty lines. Do not begin class or method implementation with empty line.

mateacademy-ai-mentor · 2026-05-10T18:05:37Z

 public class StorageImpl<K, V> implements Storage<K, V> {
+
+    public static final int MAX_SIZE = 10;
+    private Object[] keys = new Object[MAX_SIZE];


Violates checklist item #2: All non-constant fields should be initialized in the constructor, not inline.

mateacademy-ai-mentor · 2026-05-10T18:05:37Z

+
+    public static final int MAX_SIZE = 10;
+    private Object[] keys = new Object[MAX_SIZE];
+    private Object[] values = new Object[MAX_SIZE];


Violates checklist item #2: All non-constant fields should be initialized in the constructor, not inline.

mateacademy-ai-mentor · 2026-05-10T18:05:37Z

+            }
+            if (keys[i] == key) {


Violates checklist item #5 (DRY): This condition duplicates the logic from lines 14 and 18. The reference check keys[i] == key is redundant when keys[i].equals(key) already handles all cases including null.

mateacademy-ai-mentor · 2026-05-10T18:05:37Z

+            }
+            if (keys[i] == null && values[i] == null) {


CRITICAL BUG: This condition is wrong. It checks keys[i] == null && values[i] == null, but after inserting a key-value pair, values[i] is set to a non-null value. On subsequent insertions, this condition will never be true, preventing new entries from being stored. Should only check if the key slot is empty: keys[i] == null.

mateacademy-ai-mentor · 2026-05-10T18:05:37Z


    @Override
    public V get(K key) {
+


Violates checklist item #1: Remove empty line before method implementation.

mateacademy-ai-mentor · 2026-05-10T18:05:37Z

+            }
+            if (keys[i] == null && key == null) {


Violates checklist item #5 (DRY): Duplicate logic from line 14. The condition keys[i] != null && keys[i].equals(key) already handles the case where both are non-null.

mateacademy-ai-mentor · 2026-05-10T18:05:38Z

    public int size() {
-        return -1;
+        int size = 0;
+


Violates checklist item #1: Remove empty line before method implementation.

mateacademy-ai-mentor

Review Summary

The implementation still has a critical issue that must be addressed. The put() method uses values[i] == null for empty slot detection, but the task requirement explicitly specifies it should check keys[i] == null. This is a functional bug - after inserting a value into a slot, that value becomes non-null, which means the method won't be able to find empty slots for subsequent insertions, breaking the core storage operation.

Critical fix needed:

// Change from:
if (values[i] == null) { ... }
// To:
if (keys[i] == null) { ... }

Medium priority: Remove the redundant keys[i] == key checks in both put() and get() methods. The keys[i].equals(key) condition already handles all cases including null key comparisons, so these duplicate reference checks are unnecessary.

Once you fix the empty slot detection condition, the implementation should be functional. The constructor initialization and general structure look correct.

✨ 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-05-10T18:29:46Z

+                return;
+            }
+
+            if (values[i] == null) {


The empty slot detection checks values[i] == null, but the task requirement specifies it should check only keys[i] == null. Change to maintain consistency with the stated requirement.

Шукає вільний слот для значення коли ключ не збігається з жодним іншим, перевіряємо values[і], перевіряємо values[i], тому що може бути значення з ключем null, @mateacademy-ai-mentor

mateacademy-ai-mentor · 2026-05-10T18:29:46Z

    @Override
    public void put(K key, V value) {
+        for (int i = 0; i < keys.length; i++) {
+            if (keys[i] != null && keys[i].equals(key) || keys[i] == key) {


The reference equality check keys[i] == key is redundant. The condition keys[i].equals(key) already handles all cases including when both are null, so this OR clause can be removed.

@mateacademy-ai-mentor строка потрібна, ввона не обробляє варіанти коли ключ = null, без неї код працює не коректно

mateacademy-ai-mentor · 2026-05-10T18:29:46Z

+            }
+            if (keys[i] == key) {
+                return (V) values[i];


This keys[i] == key check is redundant - the first condition on line 34 already handles null keys and equal keys via keys[i].equals(key). This duplicate check should be removed.

@mateacademy-ai-mentor ця стрічка потрібна, вона перевіряє ключ null, в рядку 34 значення порівнюються лише якщо keys[i] != null, щоб уникнути помилки NullPointerExceptions

mateacademy-ai-mentor

Review Summary

Educational Override: This solution demonstrates solid understanding of array-based storage with proper use of generics and the MAX_SIZE constant. However, there are two critical bugs that must be addressed for correct functionality: 1) In put(), the empty slot check uses values[i] == null instead of keys[i] == null — this breaks when storing null values since they'll be treated as empty slots; 2) The keys[i] == key reference checks in both put() and get() are redundant since equals() already handles null safely. A minor DRY improvement would be extracting a findKeyIndex() helper method, but this is optional. You're on the right track — fixing the critical bugs will make this implementation fully correct!

✨ 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-05-11T13:03:02Z

+            /*Шукає вільний слот для значення коли ключ не збігається з жодним іншим,
+            перевіряємо values[i]
+            перевіряємо values[i], тому що може бути значення з ключем null*/
+            if (values[i] == null) {


CRITICAL BUG: Empty slot detection checks values[i] == null instead of keys[i] == null. This breaks when storing null values—they'll be treated as empty slots and overwritten. Change to if (keys[i] == null) to properly detect unused positions.

mateacademy-ai-mentor · 2026-05-11T13:03:02Z

+            перевіряємо values[i]
+            перевіряємо values[i], тому що може бути значення з ключем null*/
+            if (values[i] == null) {
+                values[i] = value;


CRITICAL BUG: After fixing the empty slot condition, this line should set keys[i] = key first, then values[i] = value. Currently if the fix isn't applied, this assigns value before the key.

mateacademy-ai-mentor · 2026-05-11T13:03:02Z

    @Override
    public void put(K key, V value) {
+        for (int i = 0; i < keys.length; i++) {
+            if (keys[i] != null && keys[i].equals(key) || keys[i] == key) {


Redundant check: The || keys[i] == key part is unnecessary. keys[i].equals(key) handles null keys correctly (returns false when keys[i] is null), so the reference equality check for null is already covered. Keep only the keys[i] != null && keys[i].equals(key) condition.

mateacademy-ai-mentor · 2026-05-11T13:03:02Z

+            if (keys[i] != null && keys[i].equals(key)) {
+                return (V) values[i];
+            }
+            if (keys[i] == key) {


Redundant check: Same issue as in put() - keys[i].equals(key) already handles null key comparison. Remove the separate keys[i] == key check.

mateacademy-ai-mentor · 2026-05-11T13:03:02Z

+        for (int i = 0; i < keys.length; i++) {
+            if (keys[i] != null && keys[i].equals(key)) {
+                return (V) values[i];
+            }
+            if (keys[i] == key) {
+                return (V) values[i];


DRY violation: The return (V) values[i] logic is duplicated in get() (lines 37 and 40). Consider extracting the index-finding logic to a private method findKeyIndex(K key) that returns the index or -1, then use it in both get() and put().

mateacademy-ai-mentor · 2026-05-11T13:03:02Z

+            /*Шукає вільний слот для значення коли ключ не збігається з жодним іншим,
+            перевіряємо values[i]
+            перевіряємо values[i], тому що може бути значення з ключем null*/


DRY violation: The comment suggests the code is explanatory but also indicates repeated logic exists. Consider a private helper method to avoid duplication between put() and get() methods.

First solution

286d204

mateacademy-ai-mentor suggested changes May 10, 2026

View reviewed changes

added fixed after review

7eb1523

Viodon requested a review from mateacademy-ai-mentor May 10, 2026 18:28

mateacademy-ai-mentor suggested changes May 10, 2026

View reviewed changes

added fixed after review v2

51a9550

Viodon requested a review from mateacademy-ai-mentor May 11, 2026 13:01

mateacademy-ai-mentor approved these changes May 11, 2026

View reviewed changes

		import core.basesyntax.Storage;

		public class StorageImpl<K, V> implements Storage<K, V> {

Conversation

Viodon commented May 10, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants