implemented a few methods#1905
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed because, although your StorageImpl correctly implements the required functionality (put, get, size with two arrays and MAX_SIZE), there are still a few checklist violations that must be fixed.
- In
src/main/java/core/basesyntax/impl/StorageImpl.java, you import and usejava.util.Objects(for example, in the equality checks insideputandget), which is explicitly disallowed; instead, create a private helper likeprivate boolean keysEqual(Object a, Object b)that does a null-safe comparison (e.g.,a == b || (a != null && a.equals(b))) and call that from both methods, then remove theObjectsimport. - In the same class constructor, the non-constant field
sizemust be explicitly initialized according to the checklist; addsize = 0;inside the constructor body even though the default is already zero. - The equality logic is currently duplicated in both
putandget; extracting that logic into the single privatekeysEqualhelper mentioned above will remove repetition and satisfy the "don’t create repeating code" requirement.
Once you address these points, your solution will both meet the functional requirements and fully comply with the checklist, and it should be ready for approval. You’ve done a solid job on the core design; treat these changes as a chance to tighten up style and adherence to the project rules.
✨ Please rate this review in the Platform's notification center1. 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 button2 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 tag3.
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.
Footnotes
| package core.basesyntax.impl; | ||
|
|
||
| import core.basesyntax.Storage; | ||
| import java.util.Objects; |
There was a problem hiding this comment.
This file imports and uses java.util.Objects which violates the checklist item: "Don't use class Objects". Replace Objects.equals(...) with a null-safe equality check (or a private helper) and remove this import.
| @Override | ||
| public void put(K key, V value) { | ||
| for (int i = 0; i < size; i++) { | ||
| if (Objects.equals(keys[i], key)) { |
There was a problem hiding this comment.
The equality check here duplicates the same logic used in get(). This violates the checklist item: "Don't create repeating code". Extract a private null-safe comparison method (for example private boolean keysEqual(Object a, Object b)) and call it from both put and get. Also avoid using Objects.equals as per the checklist.
| private Object[] values; | ||
| private int size; | ||
|
|
||
| public StorageImpl() { |
There was a problem hiding this comment.
All non-constant fields must be initialized in the constructor per the checklist: "All non-constant fields should be initialized in the constructor". size is declared but not explicitly initialized in the constructor — add size = 0; (or set it explicitly) inside the constructor.
| @Override | ||
| public V get(K key) { | ||
| for (int i = 0; i < size; i++) { | ||
| if (Objects.equals(key, keys[i])) { |
There was a problem hiding this comment.
This equality check uses Objects.equals(...), which is disallowed by the checklist: "Don't use class Objects". Use the same private null-safe comparison helper suggested above instead of calling Objects.equals here.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it correctly implements the Storage<K, V> interface and fully meets the task requirements. You use two parallel arrays with a MAX_SIZE constant of 10, handle put with proper replacement when a key already exists, and maintain the current element count via a size field that size() returns. All non-constant fields are initialized in the constructor, and you’ve avoided java.util.Objects as required. The extraction of the key-comparison logic into a private keysEqual helper nicely removes duplication and keeps the code readable. I don’t see any critical or blocking issues that would prevent this class from functioning correctly in the intended scenarios. Well done on addressing previous review points and producing a clean, requirement-compliant implementation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.