Maksym Gorodnychyi by MaksymGorodnychyi · Pull Request #1910 · mate-academy/jv-storage

MaksymGorodnychyi · 2026-04-30T09:02:37Z

No description provided.

mateacademy-ai-mentor

Review Summary

The test logs reveal critical functionality failures - all 7 tests in StorageImplTest are failing because size() returns -1 instead of the correct value, and get(key) returns null when it should return the stored value. These failures indicate the implementation is incomplete or broken. Additionally, the review identified two checklist violations: the key comparison logic keys[i] == null ? key == null : keys[i].equals(key) is duplicated in both put() and get() methods, violating the DRY principle [CHECKLIST ITEM #5]; and there is an empty line after the package declaration, which violates the 'no empty lines at start of class' rule [CHECKLIST ITEM #1]. You must fix the core storage functionality first to ensure all methods work correctly, then extract the duplicate key comparison into a private helper method to eliminate the code repetition.

✨ 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-04-30T09:13:28Z

+    @Override
+    public void put(K key, V value) {
+        for (int i = 0; i < size; i++) {
+            boolean keysAreEqual = keys[i] == null ? key == null : keys[i].equals(key);


This violates checklist item #5 (DRY principle). The key comparison logic keys[i] == null ? key == null : keys[i].equals(key) is repeated identically in get() method at line 32. Extract this to a private helper method to avoid code duplication.

mateacademy-ai-mentor · 2026-04-30T09:13:28Z

@@ -0,0 +1,46 @@
+package core.basesyntax;
+


Remove this empty line. Per checklist item #1, do not begin class implementation with empty line.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are still a few checklist violations that are considered required, not just cosmetic. The size field (e.g., in StorageImpl.java, around its declaration) must be explicitly initialized in the constructor (this.size = 0;) to comply with the rule that all non-constant fields are set there [CHECKLIST ITEM #2]. The key-comparison logic keys[i] == null ? key == null : keys[i].equals(key) is duplicated in both put() and get(); extract this into a private helper like private boolean areKeysEqual(K a, K b) and call it from both methods to avoid repetition [CHECKLIST ITEM #5]. You also need to remove the extra blank line immediately after the package (and after imports, if present) so the class declaration starts without a leading empty line [CHECKLIST ITEM #1]. Finally, the constructor should avoid creating generic arrays via (K[]) new Object[MAX_SIZE] and (V[]) new Object[MAX_SIZE]; instead, use an approach that does not rely directly on Object (for example, Array.newInstance or a pattern approved in your course) [CHECKLIST ITEM #6]. Once these issues are fixed, your implementation already has the correct put, get, and size() behavior and should be in good shape to pass both tests and checklist review.

✨ 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-04-30T09:41:17Z

 import core.basesyntax.Storage;

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


This violates checklist item #1: "Don't begin class or method implementation with empty line. Remove all redundant empty lines, be careful :)" Remove the empty line after the package/import block so the class declaration and imports start without leading blank lines.

mateacademy-ai-mentor · 2026-04-30T09:41:17Z

+    private int size;
+
+    @SuppressWarnings("unchecked")
+    public StorageImpl() {


This violates checklist item #2: "All non-constant fields should be initialized in the constructor". The field size is declared but not explicitly initialized in the constructor. Initialize it in the constructor (for example this.size = 0;) so all non-constant fields are set there.

mateacademy-ai-mentor · 2026-04-30T09:41:17Z

+            boolean keysAreEqual = keys[i] == null ? key == null : keys[i].equals(key);
+            if (keysAreEqual) {
+                values[i] = value;
+                return;
+            }
+        }
+        keys[size] = key;
+        values[size] = value;
+        size++;
    }

    @Override
    public V get(K key) {
+        for (int i = 0; i < size; i++) {
+            boolean keysAreEqual = keys[i] == null ? key == null : keys[i].equals(key);


This violates checklist item #5: "Don't create repeating code If the logic of your code repeats - move it to the separate private method." The key-comparison expression keys[i] == null ? key == null : keys[i].equals(key) is duplicated in put() and get(). Extract it into a private helper, e.g. private boolean areKeysEqual(K a, K b) { return a == null ? b == null : a.equals(b); }, and call that from both places to remove repetition.

mateacademy-ai-mentor · 2026-04-30T09:41:17Z

+        keys = (K[]) new Object[MAX_SIZE];
+        values = (V[]) new Object[MAX_SIZE];


This violates checklist item #6: "Don't use class Objects". The constructor creates arrays via (K[]) new Object[MAX_SIZE] and (V[]) new Object[MAX_SIZE] (lines shown). The checklist forbids using Object class directly here; consider using java.lang.reflect.Array.newInstance or another approach accepted by the task to create generic arrays, or clarify the permitted pattern.

mateacademy-ai-mentor · 2026-04-30T09:41:17Z

@@ -0,0 +1,46 @@
+package core.basesyntax;
+


This violates checklist item #1: "Don't begin class or method implementation with empty line. Remove all redundant empty lines, be careful :)" — remove the empty line immediately after the package declaration so the import/class declaration starts right after the package line.

mateacademy-ai-mentor · 2026-04-30T09:41:17Z

+    private int size;
+
+    @SuppressWarnings("unchecked")
+    public StorageImpl() {
+        keys = (K[]) new Object[MAX_SIZE];
+        values = (V[]) new Object[MAX_SIZE];
+    }


This violates checklist item #2: "All non-constant fields should be initialized in the constructor". The field size (declared here) is not explicitly initialized in the constructor. Initialize size in the constructor (for example this.size = 0;) to satisfy the requirement.

mateacademy-ai-mentor · 2026-04-30T09:41:17Z

+            boolean keysAreEqual = keys[i] == null ? key == null : keys[i].equals(key);
+            if (keysAreEqual) {
+                values[i] = value;
+                return;
+            }
+        }
+        keys[size] = key;
+        values[size] = value;
+        size++;
+    }
+
+    @Override
+    public V get(K key) {
+        for (int i = 0; i < size; i++) {
+            boolean keysAreEqual = keys[i] == null ? key == null : keys[i].equals(key);
+            if (keysAreEqual) {


This violates checklist item #5: "Don't create repeating code If the logic of your code repeats - move it to the separate private method." The key-comparison expression keys[i] == null ? key == null : keys[i].equals(key) is duplicated in put() and get(). Extract it to a private helper, e.g. private boolean areKeysEqual(K a, K b) { return a == null ? b == null : a.equals(b); }, and call it from both places to remove duplication.

mateacademy-ai-mentor · 2026-04-30T09:41:18Z

+        keys = (K[]) new Object[MAX_SIZE];
+        values = (V[]) new Object[MAX_SIZE];


This violates checklist item #6: "Don't use class Objects". The constructor creates generic arrays via (K[]) new Object[MAX_SIZE] and (V[]) new Object[MAX_SIZE]. Avoid direct use of Object here per the checklist — consider using java.lang.reflect.Array.newInstance(...) or another permitted pattern to create generic arrays without relying on raw Object array creation.

mateacademy-ai-mentor

Review Summary

Your implementation demonstrates good progress with the core Storage interface requirements, including proper put(), get(), and size() methods using generics. However, there are a few checklist items to address before final approval: [CHECKLIST ITEM #2] Initialize the size field explicitly with this.size = 0; in the constructor. [CHECKLIST ITEM #5] Extract the duplicated key comparison logic keys[i] == null ? key == null : keys[i].equals(key) into a private helper method like private boolean areKeysEqual(K a, K b) to follow DRY principles. [CHECKLIST ITEM #6] Replace (K[]) new Object[MAX_SIZE] with Array.newInstance() for proper generic array creation. This is an educational approval to help you progress while learning these important patterns—keep up the good 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-04-30T10:00:31Z

+    private Entry<K, V>[] entries;
+    private int size;
+
+    private static class Entry<K, V> {


Checklist item #2 violation: The 'size' field must be explicitly initialized in the constructor. Add 'this.size = 0;' inside the constructor.

mateacademy-ai-mentor · 2026-04-30T10:00:31Z

+            this.key = key;
+            this.value = value;
+        }
+    }


Checklist item #5 violation: Key comparison logic is duplicated in both put() and get() methods. Extract to a private helper method like 'private boolean areKeysEqual(K a, K b)' to follow DRY principle.

mateacademy-ai-mentor · 2026-04-30T10:00:32Z

+                    ? key == null
+                    : entries[i].key.equals(key);
+            if (keysAreEqual) {
+                entries[i].value = value;


Checklist item #5 violation: This same key comparison logic is duplicated from put(). Extract to a private helper method.

mateacademy-ai-mentor · 2026-04-30T10:00:32Z

+    private int size;
+
+    private static class Entry<K, V> {
+        private K key;


Checklist item #6 violation: Avoid creating generic arrays via direct Object[] cast. Use Array.newInstance() or another approach that doesn't rely directly on Object.

mateacademy-ai-mentor · 2026-04-30T10:00:32Z

+
+    private static class Entry<K, V> {
+        private K key;
+        private V value;


Checklist item #6 violation: Same issue as line 11 - avoid direct Object[] cast for generic arrays.

mateacademy-ai-mentor · 2026-04-30T10:00:32Z


    @Override
    public V get(K key) {
+        for (int i = 0; i < size; i++) {


Checklist item #5 violation: Same key comparison logic duplicated in get() method. Extract to the same private helper method.

mateacademy-ai-mentor · 2026-04-30T10:00:32Z

+    }
+
+    @SuppressWarnings("unchecked")
+    public StorageImpl() {


Checklist item #2 violation: The 'size' field should be explicitly initialized in the constructor. Add 'this.size = 0;' inside the constructor.

mateacademy-ai-mentor · 2026-04-30T10:00:32Z

+    private static final int MAX_SIZE = 10;
+    private K[] keys;
+    private V[] values;
+    private int size;


CHECKLIST ITEM #2 VIOLATION: The size field is declared on line 7 but not explicitly initialized in the constructor. Add this.size = 0; in the constructor.

mateacademy-ai-mentor · 2026-04-30T10:00:32Z

+            boolean keysAreEqual = keys[i] == null ? key == null : keys[i].equals(key);
+            if (keysAreEqual) {
+                values[i] = value;
+                return;
+            }
+        }
+        keys[size] = key;
+        values[size] = value;
+        size++;
+    }
+
+    @Override
+    public V get(K key) {
+        for (int i = 0; i < size; i++) {
+            boolean keysAreEqual = keys[i] == null ? key == null : keys[i].equals(key);


CHECKLIST ITEM #5 VIOLATION: The key comparison logic keys[i] == null ? key == null : keys[i].equals(key) is duplicated in both put() (line 18) and get() (line 32). Extract this into a private helper method like private boolean areKeysEqual(K a, K b) to avoid repetition.

mateacademy-ai-mentor · 2026-04-30T10:00:32Z

+        keys = (K[]) new Object[MAX_SIZE];
+        values = (V[]) new Object[MAX_SIZE];


CHECKLIST ITEM #6 VIOLATION: Generic arrays should not be created via direct (K[]) new Object[] cast. Use Array.newInstance approach instead for both keys (line 11) and values (line 12) arrays.

Elena-Bruyako

see comments

Elena-Bruyako · 2026-04-30T19:33:19Z

+    private Entry<K, V>[] entries;
+    private int size;
+
+    private static class Entry<K, V> {


inner class should be placed at the end of outer class

Elena-Bruyako · 2026-04-30T19:34:19Z

@@ -0,0 +1,46 @@
+package core.basesyntax;
+
+public class StorageImpl<K, V> implements Storage<K, V> {


Why there is 2 StorageImpl classes?

Elena-Bruyako · 2026-04-30T19:36:30Z


    @Override
    public V get(K key) {
+        for (int i = 0; i < size; i++) {


create

private int indexOf(K key) { for (int i = 0; i < size; i++) { if (key == keys[i] || key != null && key.equals(keys[i])) { return i; } } return NO_INDEX; }

Suggested change

for (int i = 0; i < size; i++) {

int index = indexOf(key);

return index == NO_INDEX ? null : values[index];

Elena-Bruyako · 2026-04-30T19:37:01Z

    @Override
    public void put(K key, V value) {
+        for (int i = 0; i < size; i++) {
+            boolean keysAreEqual = entries[i].key == null


think how to modify method according to my previous comment

Maksym Gorodnychyi

68d5332

mateacademy-ai-mentor suggested changes Apr 30, 2026

View reviewed changes

MaksymGorodnychyi added 3 commits April 30, 2026 12:22

Maksym Gorodnychyi

cdcac41

Merge remote-tracking branch 'origin/master'

a127230

Maksym Gorodnychyi

ee7942e

MaksymGorodnychyi requested a review from mateacademy-ai-mentor April 30, 2026 09:36

mateacademy-ai-mentor suggested changes Apr 30, 2026

View reviewed changes

MaksymGorodnychyi added 2 commits April 30, 2026 12:55

Maksym Gorodnychyi

6b53c3c

Maksym Gorodnychyi

a360d0b

MaksymGorodnychyi requested a review from mateacademy-ai-mentor April 30, 2026 09:58

mateacademy-ai-mentor approved these changes Apr 30, 2026

View reviewed changes

Elena-Bruyako suggested changes Apr 30, 2026

View reviewed changes

		keys = (K[]) new Object[MAX_SIZE];
		values = (V[]) new Object[MAX_SIZE];

		@@ -0,0 +1,46 @@
		package core.basesyntax;

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

	for (int i = 0; i < size; i++) {
	int index = indexOf(key);
	return index == NO_INDEX ? null : values[index];

Conversation

MaksymGorodnychyi commented Apr 30, 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!

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!

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!

Elena-Bruyako left a comment

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!

3 participants