Storage - first solution by mate-student26 · Pull Request #1896 · mate-academy/jv-storage

mate-student26 · 2026-03-25T08:42:33Z

No description provided.

mateacademy-ai-mentor

Review Summary

Great job implementing the core functionality of the Storage class. Your solution correctly handles adding, retrieving, and updating key-value pairs. However, changes are needed because a key checklist item was not met. In StorageImpl.java, the logic for finding a key's index is duplicated in both the put() and get() methods, which violates the 'Don't create repeating code' rule [CHECKLIST ITEM #5]. Please refactor this logic into a private helper method to eliminate the duplication. After you've addressed this, I'll be happy to take another look!

✨ 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-25T08:44:11Z

+        for (int i = 0; i < size; i++) {
+            if (Objects.equals(key,keys[i])) {
+                values[i] = value;
+                return;
+            }
+        }


This loop for finding an existing key is very similar to the one in the get method (lines 35-39). This violates checklist item #5: 'Don't create repeating code'. Consider extracting this logic into a private helper method that finds the index of a key. You can then call that method from both put() and get() to avoid duplication.

mateacademy-ai-mentor · 2026-03-25T08:44:11Z

+
+import core.basesyntax.impl.StorageImpl;
+
+public class Main {


This import is unused and can be safely removed.

mateacademy-ai-mentor · 2026-03-25T08:44:11Z

+        storage.put(33, box);
+        storage.size();
+
+        System.out.println(storage.get(33));


This method call has no effect because its return value is not assigned to any variable or used in any way. You can remove this line.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
_{[CODE: NCM]}

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
_{[CODE: NCM]}

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
_{[CODE: NCM]}

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
_{[CODE: NCM]}

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

slade13

The implementation is generally correct, but it uses Objects.equals(), which is explicitly forbidden in this task. Please replace this comparison with manual null-safe key comparison logic. After that, the solution should be fine.

slade13 · 2026-03-26T10:07:38Z

 package core.basesyntax.impl;

 import core.basesyntax.Storage;
+import java.util.Objects;


Your implementation uses java.util.Objects, this directly violates the task checklist requirement:

Don't use class Objects

slade13 · 2026-03-26T10:07:41Z

    @Override
    public void put(K key, V value) {
+        for (int i = 0; i < size; i++) {
+            if (Objects.equals(key,keys[i])) {


There should be a space after the comma:

Suggested change

if (Objects.equals(key,keys[i])) {

if (Objects.equals(key, keys[i])) {

slade13

Your code does not build now: https://github.com/mate-academy/jv-storage/actions/runs/23595932059/job/68712557766?pr=1896

Error: Failures:
Error: StorageImplTest.addTwoElementsWithSameKey:140 With two elements added with the same key, the value should be rewritten expected:<[Four]> but was:<[Two]>
Error: StorageImplTest.getElementWhenKeyIsObject:118 Test failed! Method get(key) should return correct value expected:<Element 2> but was:
Error: Errors:
Error: StorageImplTest.addTwoElementsWithNullKey:155 » NullPointer Key or value cannot be null.
Error: StorageImplTest.getElementWithKeyNull:82 » NullPointer Key or value cannot be null.
[INFO]
Error: Tests run: 8, Failures: 2, Errors: 2, Skipped: 0

The implementation has two functional issues. First, it rejects null keys, while this task expects null key support. Second, keys are compared with == instead of logical equality, so equal objects are not treated as the same key. Because of that, retrieving and replacing values by equal object keys does not work correctly.

mateuszwojtkowiak

Great job! The only thing that could be improved is repeated logic in two methods. The task is accepted, but please read my comment and create a helper method to avoid duplication.

mateuszwojtkowiak · 2026-05-04T14:10:29Z

    @Override
    public void put(K key, V value) {
+
+        for (int i = 0; i < size; i++) {


You have duplicated key‑searching logic in both put() and get().
This violates the “Don’t repeat yourself” rule from the checklist.

Please extract this repeated logic into a private helper method, for example:

private int indexOfKey(K key) { ... }

Then call this method from both put() and get() to avoid code duplication.

Brynkiewicz added 2 commits March 25, 2026 09:42

Storage - first solution

90da774

Storage - first checkstyle changes

ecf1d38

mateacademy-ai-mentor suggested changes Mar 25, 2026

View reviewed changes

mate-student26 requested a review from mateacademy-ai-mentor March 25, 2026 08:44

mateacademy-ai-mentor reviewed Mar 25, 2026

View reviewed changes

slade13 suggested changes Mar 26, 2026

View reviewed changes

Brynkiewicz added 2 commits March 26, 2026 14:02

Storage - comparisation method change

05e423b

Storage - comparisation method change; null checking

07b7b00

mate-student26 requested a review from slade13 March 26, 2026 13:14

slade13 suggested changes Mar 26, 2026

View reviewed changes

null keys checking changed in put and get method

88e810c

mate-student26 requested a review from slade13 May 4, 2026 13:12

mateuszwojtkowiak approved these changes May 4, 2026

View reviewed changes

	if (Objects.equals(key,keys[i])) {
	if (Objects.equals(key, keys[i])) {

Conversation

mate-student26 commented Mar 25, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

mateacademy-ai-mentor Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

slade13 left a comment

Choose a reason for hiding this comment

Uh oh!

slade13 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

slade13 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

slade13 left a comment

Choose a reason for hiding this comment

Uh oh!

mateuszwojtkowiak left a comment

Choose a reason for hiding this comment

Uh oh!

mateuszwojtkowiak May 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants