Solution#1921
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation correctly fulfills all core functional requirements including generic Storage<K,V> interface, put/get/size methods with proper key replacement logic, and array-based storage with MAX_SIZE=10. No collections were used as required. The main issues identified are non-blocking concerns: the duplicate StorageImpl.java file in the wrong package can be simply deleted; the null-safe comparison logic works correctly due to Java's short-circuit evaluation (key != null && key.equals(keys[i])), but extracting this into a private findKeyIndex(K key) helper method would improve code organization per DRY principles; empty lines between methods in Application.java are a minor style preference. Optional improvements to consider: delete src/main/java/core/basesyntax/StorageImpl.java since the correct implementation is in the impl subpackage; extract the duplicated key-searching loop into a reusable private method for cleaner code; remove any blank lines between method implementations in Application.java. Your solution is approved and meets the task requirements. The core functionality works correctly - these are code quality suggestions rather than blockers.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| @Override | ||
| public void put(K key, V value) { | ||
| for (int i = 0; i < size; i++) { | ||
| if (key == keys[i] || (key != null && key.equals(keys[i]))) { |
There was a problem hiding this comment.
This will throw NullPointerException if key is null. The expression key == keys[i] is evaluated first, and if key is null, it becomes null == keys[i], but if keys[i] is also null, Java still evaluates the left side as false. However, if keys[i] is not null, calling .equals() on null throws NPE. Use null-safe comparison: key == null && keys[i] == null || key != null && key.equals(keys[i])
| @Override | ||
| public V get(K key) { | ||
| for (int i = 0; i < size; i++) { | ||
| if (key == keys[i] || (key != null && key.equals(keys[i]))) { |
There was a problem hiding this comment.
This has the same null-handling issue as line 20. When key is null, key.equals(keys[i]) will throw NPE.
| for (int i = 0; i < size; i++) { | ||
| if (key == keys[i] || (key != null && key.equals(keys[i]))) { | ||
| values[i] = value; | ||
| return; | ||
| } | ||
| } | ||
| if (size < MAX_SIZE) { | ||
| keys[size] = key; | ||
| values[size] = value; | ||
| size++; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public V get(K key) { | ||
| for (int i = 0; i < size; i++) { | ||
| if (key == keys[i] || (key != null && key.equals(keys[i]))) { |
There was a problem hiding this comment.
The null-checking logic in put() and get() methods is identical. Per DRY principle, extract this into a private helper method like findKeyIndex(K key) to avoid code duplication.
| package core.basesyntax; | ||
|
|
||
| public class StorageImpl<K, V> implements Storage<K, V> { |
There was a problem hiding this comment.
Missing import statement. This file cannot implement Storage interface without importing it. Add: import core.basesyntax.Storage;
| values[i] = value; | ||
| return; | ||
| } | ||
| } | ||
| if (size < MAX_SIZE) { | ||
| keys[size] = key; | ||
| values[size] = value; | ||
| size++; | ||
| } | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
The key-searching loop logic is duplicated in the get() method. Consider extracting this into a private helper method like findKeyIndex(K key) to follow DRY principle (checklist item #5).
| if (key == null && keys[i] == null || key != null && key.equals(keys[i])) { | ||
| return values[i]; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
The key-searching loop logic duplicates code from put() method. Extract to a private helper method to follow DRY principle (checklist item #5).
| System.out.println("Storage size: " + storage.size()); | ||
|
|
||
| storage.put(1, "NewValue1"); | ||
| System.out.println("Get updated value by key 1: " + storage.get(1)); |
There was a problem hiding this comment.
Per checklist requirements, remove empty lines between method implementations. There should be no blank line after line 15 before the next method.
| import core.basesyntax.impl.StorageImpl; | ||
|
|
||
| public class Application { | ||
| public static void main(String[] args) { |
There was a problem hiding this comment.
The constant name MAX_SIZE is not fully informative. Consider renaming to MAX_STORAGE_CAPACITY to clearly indicate its purpose.
2 participants
No description provided.