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 several core requirements of the custom ArrayList are not yet met and these will cause tests to fail or runtime errors. The backing array never grows when full, so methods like add(T value) and add(T value, int index) can throw ArrayIndexOutOfBoundsException instead of automatically resizing by 1.5× via a dedicated private resize/ensureCapacity method. Index-based methods get, set, and remove(int index) do not use a shared index-check helper and therefore can throw low-level array exceptions instead of the required ArrayListIndexOutOfBoundsException with clear messages. The addAll(List<T> list) method currently replaces the internal storage and uses an uninitialized capacity field, instead of appending the new elements to the end after calling the resize helper to ensure enough space. In remove(T element), comparing with elements[i].equals(element) is not null-safe and will throw NullPointerException for stored nulls, which is not consistent with java.util.ArrayList behavior. Please also replace all manual element-shifting loops (in add(index), addAll, remove(index)) with System.arraycopy(), remove unused fields like capacity and loadFactor, and simplify isEmpty() to directly return size == 0. Once these functional and checklist-related issues are addressed, your implementation will be much closer to a correct, robust ArrayList clone.
✨ 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
| @Override | ||
| public void add(T value) { |
There was a problem hiding this comment.
The add(T value) method inserts without ensuring capacity or resizing. The task requires the internal array to grow 1.5× and that resizing be placed in a separate method ("Resize the array in a separate method."). Implement a resize helper and call it before inserting.
| } | ||
| for (int i = size - 1; i >= index; i--) { |
There was a problem hiding this comment.
add(T value, int index) shifts elements using a manual loop instead of System.arraycopy(). The checklist requires: "Use System.arraycopy() to move your array elements." Replace the loop with System.arraycopy() and ensure capacity before shifting.
| @Override | ||
| public void add(T value, int index) { | ||
|
|
||
| if (index < 0 || index > size) { | ||
| throw new ArrayListIndexOutOfBoundsException(String.format("Index %d is out of bounds for ArrayList with size %d", index, size)); | ||
| } | ||
| for (int i = size - 1; i >= index; i--) { | ||
| elements[i + 1] = elements[i]; | ||
| } | ||
| elements[index] = value; |
There was a problem hiding this comment.
add(T value, int index) does not ensure capacity before insertion. The implementation must resize when full (see description: internal array should grow 1.5 times when it is full) and resizing logic must be in a separate method ("Resize the array in a separate method.").
| @Override | ||
| public void addAll(List<T> list) { | ||
|
|
||
| elements = (T[]) new Object[list.size() + capacity]; | ||
| for (int i = 0; i < list.size(); i++) { | ||
| elements[i] = list.get(i); | ||
| } |
There was a problem hiding this comment.
addAll(List list) replaces the internal storage and sets size to list.size(), discarding existing elements; it also uses the field capacity which is never initialized. This is incorrect — addAll should append elements (or clearly replace), and you must not rely on an uninitialized/unused field. Also follow: "Resize the array in a separate method."
| @Override | ||
| public T get(int index) { |
There was a problem hiding this comment.
get(int index) returns elements[index] without any index validation or throwing the required custom exception. The checklist advises centralizing repeated checks: "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." Implement a shared index-check helper that throws informative ArrayListIndexOutOfBoundsException when index is invalid.
| @Override | ||
| public T remove(T element) { | ||
| return null; | ||
| for (int i = 0; i < size; i++) { | ||
| if (elements[i].equals(element)) { |
There was a problem hiding this comment.
remove(T element) compares via elements[i].equals(element) which will throw NullPointerException for stored null elements; java.util.ArrayList supports null elements. Make comparisons null-safe (e.g., elements[i] == null ? element == null : elements[i].equals(element)) so your behavior matches the description's expectation to behave like java.util.ArrayList. The method correctly throws NoSuchElementException when not found.
| private int capacity; | ||
| private int loadFactor; |
There was a problem hiding this comment.
Two fields are declared but never initialized/used: private int capacity; and private int loadFactor;. This violates the checklist: "Don't create redundant variables." Remove or properly use these fields.
| } | ||
| for (int i = size - 1; i >= index; i--) { | ||
| elements[i + 1] = elements[i]; | ||
| } | ||
| elements[index] = value; | ||
| size++; | ||
| } | ||
|
|
||
| @Override | ||
| public void addAll(List<T> list) { | ||
|
|
||
| elements = (T[]) new Object[list.size() + capacity]; | ||
| for (int i = 0; i < list.size(); i++) { |
There was a problem hiding this comment.
There are multiple manual loops used to move/copy elements (add(index), addAll, remove(index)). The checklist requires: "Use System.arraycopy() to move your array elements." Replace the manual loops at these places with System.arraycopy() for correctness and performance.
|
|
||
| @Override | ||
| public boolean isEmpty() { | ||
| return false; | ||
| if (size == 0) { | ||
| return true; |
There was a problem hiding this comment.
isEmpty() uses an unnecessary if-else where a single boolean return would suffice; this violates: "Don't complicate if-else construction." Simplify to return size == 0;.
| @Override | ||
| public T get(int index) { | ||
| return null; | ||
| return elements[index]; | ||
| } | ||
|
|
||
| @Override | ||
| public void set(T value, int index) { | ||
|
|
||
| elements[index] = value; | ||
| } | ||
|
|
||
| @Override | ||
| public T remove(int index) { | ||
| return null; | ||
| T removedElement = elements[index]; | ||
| for (int i = index; i < size - 1; i++) { | ||
| elements[i] = elements[i + 1]; | ||
| } | ||
| elements[--size] = null; |
There was a problem hiding this comment.
Some index-related methods currently can throw low-level ArrayIndexOutOfBoundsException rather than the required custom ArrayListIndexOutOfBoundsException with an informative message. The description mandates: "Throw custom ArrayListIndexOutOfBoundsException in case the index passed to any of the methods is invalid." Ensure all index checks throw that custom exception with clear messages.
2 participants
No description provided.