added solution #1590
added solution #1590
Conversation
There was a problem hiding this comment.
It looks like there are a few areas that need some attention, particularly with the way keys are being checked and compared. Using == for key comparison might lead to unexpected behavior since it doesn't check for logical equality. Extracting repeated logic into a private method is also a great idea to keep your code clean and maintainable. Keep up the good work, and with a few tweaks, your code will be even better! 👍 Remember, clean and maintainable code is just as important as working code!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @Override | ||
| public void put(K key, V value) { | ||
| for (int i = 0; i < currentSize; i++) { |
There was a problem hiding this comment.
There is repeating code for checking if the key is already in the storage. Consider extracting this logic into a private method to adhere to the DRY principle.
| } | ||
|
|
||
| @Override | ||
| public V get(K key) { | ||
| for (int i = 0; i < currentSize; i++) { |
There was a problem hiding this comment.
The same repeating code for checking if the key is already in the storage is found here as well. It's a good practice to create a private method for such repeated logic.
| @Override | ||
| public void put(K key, V value) { | ||
| for (int i = 0; i < currentSize; i++) { | ||
| if (key == null && keys[i] == null) { |
There was a problem hiding this comment.
The comparison of keys using '==' does not account for the potential logical equality of key objects. It is better to use 'equals' method with a null check.
| } | ||
|
|
||
| for (int i = 0; i < currentSize; i++) { | ||
| if (keys[i] != null && keys[i].equals(key)) { |
There was a problem hiding this comment.
The null check for 'keys[i]' is not necessary here as it's already handled by the previous loop (lines 19-23).
| } | ||
|
|
||
| @Override | ||
| public V get(K key) { | ||
| for (int i = 0; i < currentSize; i++) { | ||
| if (key == null && keys[i] == null) { |
There was a problem hiding this comment.
As mentioned before, consider using 'equals' method with a null check for comparing keys instead of '==', to ensure logical equality is considered.
| } | ||
|
|
||
| for (int i = 0; i < currentSize; i++) { | ||
| if (keys[i] != null && keys[i].equals(key)) { |
There was a problem hiding this comment.
Similar to the previous comment, the null check for 'keys[i]' is not necessary here as it's already handled by the previous loop (lines 54-58).
There was a problem hiding this comment.
Great job on the Pull Request! All the feedback provided is related to minor code style issues, such as removing unnecessary empty lines. These small tweaks don't impact the functionality or security of your code, so your PR is approved! 🎉 Remember to keep an eye on these details in the future for even cleaner code. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @@ -1,19 +1,54 @@ | |||
| package core.basesyntax.impl; | |||
|
|
|||
| import core.basesyntax.Storage; | |||
| import java.util.Objects; | |||
|
|
|||
| public class StorageImpl<K, V> implements Storage<K, V> { | |||
There was a problem hiding this comment.
Don't begin class or method implementation with an empty line. Please remove the redundant empty line at the beginning of the class definition.
| private final V[] values; | ||
| private int currentSize; | ||
|
|
||
| public StorageImpl() { |
There was a problem hiding this comment.
Don't begin class or method implementation with an empty line. Please remove the redundant empty line at the beginning of the constructor.
| values = (V[]) new Object[MAX_ARRAY_SIZE]; | ||
| currentSize = 0; | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
Don't begin class or method implementation with an empty line. Please remove the redundant empty line at the beginning of the put method.
| currentSize++; | ||
| } else { | ||
| throw new RuntimeException("Storage is full"); | ||
| } | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
Don't begin class or method implementation with an empty line. Please remove the redundant empty line at the beginning of the get method.
| int index = findKeyIndex(key); | ||
| if (index != -1) { | ||
| return values[index]; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
Don't begin class or method implementation with an empty line. Please remove the redundant empty line at the beginning of the size method.
| return currentSize; | ||
| } | ||
|
|
||
| private int findKeyIndex(K key) { |
There was a problem hiding this comment.
Don't begin class or method implementation with an empty line. Please remove the redundant empty line at the beginning of the findKeyIndex method.
| for (int i = 0; i < currentSize; i++) { | ||
| if ((keys[i] == null && key == null) || Objects.equals(keys[i], key)) { | ||
| return i; | ||
| } |
There was a problem hiding this comment.
Consider using the Objects.equals method directly without checking for null separately, as Objects.equals already handles nulls. This will make the code cleaner.
| public StorageImpl() { | ||
| keys = (K[]) new Object[MAX_ARRAY_SIZE]; | ||
| values = (V[]) new Object[MAX_ARRAY_SIZE]; | ||
| currentSize = 0; |
There was a problem hiding this comment.
| currentSize = 0; |
| private static final int MAX_ARRAY_SIZE = 10; | ||
| private final K[] keys; | ||
| private final V[] values; | ||
| private int currentSize; |
There was a problem hiding this comment.
| private int currentSize; | |
| private int size; |
|
|
||
| private int findKeyIndex(K key) { | ||
| for (int i = 0; i < currentSize; i++) { | ||
| if ((keys[i] == null && key == null) || Objects.equals(keys[i], key)) { |
There was a problem hiding this comment.
| if ((keys[i] == null && key == null) || Objects.equals(keys[i], key)) { | |
| if (keys[i] == key || Objects.equals(keys[i], key)) { |
don't use Objects
No description provided.