Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still two direct checklist violations that need to be addressed before this task is considered complete. Functionally your custom ArrayList works well: it grows correctly by 1.5×, uses System.arraycopy, throws the right exceptions, and mostly matches java.util.ArrayList behavior. However, in src/main/java/core/basesyntax/ArrayList.java your grow() method still uses the literal 1.5 instead of a named constant, which breaks the “any magic numbers should be constants” rule [CHECKLIST ITEM #3]; please introduce something like private static final double GROWTH_FACTOR = 1.5; and use it there. Also, the index bounds checks are duplicated across get, set, and remove(int), and the insertion bounds check in add(T value, int index) is also inlined; the checklist explicitly requires moving such repeated logic into private helper methods [CHECKLIST ITEM #5]. To fix this, extract methods like private void checkElementIndex(int index) for 0 <= index < size and private void checkPositionIndex(int index) for 0 <= index <= size, and call them from the relevant public methods instead of repeating the conditions. Once you extract the growth-factor constant and centralize the index checks, your solution will fully meet both the functional requirements and the checklist expectations, and the code will be easier to maintain and read.
✨ 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
| private int size; | ||
|
|
||
| private void grow() { | ||
| int newCapacity = (int) (elementData.length * 1.5); |
There was a problem hiding this comment.
This line uses the literal growth factor 1.5. The checklist requires moving hardcoded values to named constants: "Any magic numbers should be constants
Your code should be easy to read. Move all hardcoded values to constant fields and give them informative names." Extract a constant like private static final double GROWTH_FACTOR = 1.5 and use it here.
| if (index < 0 || index >= size) { | ||
| throw new ArrayListIndexOutOfBoundsException("Index: " + index + ", Size: " + size); | ||
| } | ||
| return (T) elementData[index]; | ||
| } | ||
|
|
||
| @Override | ||
| public void set(T value, int index) { | ||
|
|
||
| if (index < 0 || index >= size) { | ||
| throw new ArrayListIndexOutOfBoundsException("Index: " + index + ", Size: " + size); | ||
| } | ||
| elementData[index] = value; | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| @Override | ||
| public T remove(int index) { | ||
| return null; | ||
| if (index < 0 || index >= size) { | ||
| throw new ArrayListIndexOutOfBoundsException("Index: " + index + ", Size: " + size); |
There was a problem hiding this comment.
Index bounds checks are repeated in get, set, and remove(int). The checklist says: "Don't create repeating code.
If the logic of your code repeats - move it to a separate private method. For example, the index checks for the get(), set(), and remove() methods should be moved to a separate method." Extract a helper (e.g., private void checkElementIndex(int index)) for 0 <= index < size and call it from these methods to satisfy the requirement and remove duplication.
| if (index < 0 || index > size) { | ||
| throw new ArrayListIndexOutOfBoundsException("Invalid index: " + index); |
There was a problem hiding this comment.
The insertion method add(T value, int index) performs a similar bounds check but with different semantics (0 <= index <= size). Consider extracting a separate helper like checkPositionIndex(int index) for insertion positions to avoid duplicated logic while preserving correct bounds for add. This supports the same checklist principle to avoid repeating code.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! I am approving your solution. You've done a great job addressing the feedback from the previous review by replacing the magic number with a GROWTH_FACTOR constant and extracting the duplicated index-checking logic into private helper methods. Your ArrayList implementation is now clean, correct, and fully meets all the requirements. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.