f#1780
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the current implementation does not compile and several core functional requirements of the custom ArrayList task are missing. In src/main/java/core/basesyntax/ArrayList.java, there is a syntax error around line 12 (elementData = ;), and the internal array elementData is never initialized—add a constructor that sets elementData = new Object[DEFAULT_SIZE]; and removes any invalid statements. You also need a private resize method that grows the backing array by 1.5x (e.g., using int newCapacity = oldCapacity + (oldCapacity >> 1);), allocates a new array, and copies elements with System.arraycopy(), then call this method from add(T value) and add(int index, T value) when the array is full. Index validation is currently absent: implement a private helper like checkIndex(int index, int upperBound) that throws your custom ArrayListIndexOutOfBoundsException with an informative message for invalid indexes, and use it in get, set, remove(int index), and add(int index, T value); also ensure remove(T element) throws NoSuchElementException when the element is not found. In ArrayList.java, methods such as add, add(int index, T value), get, set, remove, addAll, and isEmpty must be fully implemented to match java.util.ArrayList behavior, using System.arraycopy() for shifting elements on insert and remove instead of manual loops. In src/main/java/core/basesyntax/Main.java, line 8 currently contains an invalid statement and the import of java.util.ArrayList shadows your own class—remove the java.util imports, ensure main contains valid code, and instantiate your list as core.basesyntax.ArrayList<Type> list = new core.basesyntax.ArrayList<>();. Once you address these compilation errors and implement the required behaviors, your solution will much better demonstrate understanding of dynamic arrays and will be in line with the task’s checklist.
✨ 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
|
|
||
| public class Main { |
There was a problem hiding this comment.
The internal storage elementData is declared but never initialized and DEFAULT_SIZE is not used. You need a constructor that initializes elementData = new Object[DEFAULT_SIZE] so the list has the required default capacity (same as java.util.ArrayList).
|
|
||
| public class Main { | ||
| public static void main(String[] args) { | ||
| ArrayList |
There was a problem hiding this comment.
Checklist requirement: "Resize the array in a separate method." There is no resize/grow method in this class. Add a private method to handle resizing, computing new capacity as oldCapacity + (oldCapacity >> 1) (1.5x) and using System.arraycopy() to copy elements.
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class Main { | ||
| public static void main(String[] args) { | ||
| ArrayList |
There was a problem hiding this comment.
Main.java imports java.util.ArrayList which will shadow your core.basesyntax.ArrayList. Also the main body is incomplete (ArrayList on its own). Remove or fix the import and instantiate your implementation correctly, e.g. core.basesyntax.ArrayList<Type> list = new core.basesyntax.ArrayList<>(); and complete the main method.
| @Override | ||
| public void add(int index, T value) { | ||
| if (elementData.length == size) { | ||
| elementData = ; |
There was a problem hiding this comment.
Syntax error: elementData = ; is an invalid assignment and prevents compilation. Implement resizing via a separate method and assign its result here (see checklist: "Resize the array in a separate method.").
| public void add(T value) { | ||
|
|
||
| private static final int DEFAULT_SIZE = 10; | ||
| private Object[] elementData; |
There was a problem hiding this comment.
The internal storage elementData is declared but never initialized (no constructor). Initialize it to new Object[DEFAULT_SIZE] in a constructor so the list has the default capacity required by the task.
| private static final int DEFAULT_SIZE = 10; | ||
| private Object[] elementData; | ||
| private int size; | ||
|
|
There was a problem hiding this comment.
There is no resize/grow helper method. The checklist explicitly requires: "Resize the array in a separate method." Add a private method that grows elementData by 1.5x (matching java.util.ArrayList behavior) and use System.arraycopy() to copy elements.
|
|
||
| @Override | ||
| public void add(int index, T value) { | ||
| if (elementData.length == size) { |
There was a problem hiding this comment.
The implementation never throws the required custom exception for invalid indexes. The description requires: "Throw custom ArrayListIndexOutOfBoundsException in case the index passed to any of the methods is invalid." Add and use this exception with an informative message in your index-validation method.
| if (elementData.length == size) { | ||
| elementData = ; | ||
| } | ||
| elementData[index] = value; |
There was a problem hiding this comment.
Overall missing use of System.arraycopy() for shifting/copying elements. Per checklist: "Use System.arraycopy() to move your array elements." Replace manual loops (or absent logic) with System.arraycopy() in insert/remove/grow operations.
| @Override | ||
| public int size() { | ||
| return 0; | ||
| return size; |
There was a problem hiding this comment.
size() is implemented correctly returning size, but other methods that rely on it are unimplemented; ensure consistency when you implement add/remove methods.
| public class ArrayList<T> implements List<T> { | ||
| @Override | ||
| public void add(T value) { | ||
|
|
||
| private static final int DEFAULT_SIZE = 10; | ||
| private Object[] elementData; | ||
| private int size; | ||
|
|
There was a problem hiding this comment.
Main.java incorrectly imports java.util.ArrayList which will shadow your core.basesyntax.ArrayList. Also main is incomplete: the line ArrayList alone is invalid. Either import/use your implementation or fully qualify the intended class and finish the example usage.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still several critical issues that break requirements and some core behaviors of your custom ArrayList. The biggest blockers are the continued use of forbidden utility classes (java.util.Arrays and java.util.Objects) in ArrayList.java and Main.java, which directly violates the task checklist; you should remove these imports and replace Arrays.copyOf, Arrays.toString, and Objects.equals with manual logic plus System.arraycopy and explicit null-safe comparisons. In ArrayList.remove(T element), the loop currently walks the entire backing array and then calls System.arraycopy with size - index elements, which is off by one for removal and can leave incorrect data; iterate only up to size, use a manual null-safe equality check, shift size - index - 1 elements, then decrement size and null out the last slot, and throw NoSuchElementException with a clear message if nothing was removed. The grow() method must not use Arrays.copyOf; instead, compute the new capacity, allocate a new Object[newCapacity], and copy existing elements with System.arraycopy(elementData, 0, newArray, 0, size), then reassign the backing array. The indexed add method currently has the wrong parameter order (add(T value, int index) instead of add(int index, T value)) and performs its own index validation instead of reusing your index-check helper; update the signature to match the required interface and call a shared helper that allows index == size for insertion at the end. Your toString() implementation should not rely on Arrays.toString nor include null values beyond size; instead, build the string manually (e.g., with StringBuilder) over indices 0 to size - 1 to match java.util.ArrayList output. Finally, make sure all thrown exceptions, especially NoSuchElementException in remove(T element), include informative messages explaining what went wrong, as required by the checklist. Once you address these issues, your solution will be much closer to a clean, fully compliant ArrayList implementation, and the work you’ve already done on resizing, basic CRUD operations, and custom exceptions will fit in nicely.
✨ 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
| @@ -1,48 +1,113 @@ | |||
| package core.basesyntax; | |||
|
|
|||
| import java.util.Arrays; | |||
There was a problem hiding this comment.
This import (java.util.Arrays) violates the checklist: "Don't use Objects, Arrays, or any other util class." Remove this import and avoid using Arrays.* methods (use manual logic + System.arraycopy where required).
|
|
||
| import java.util.Arrays; | ||
| import java.util.NoSuchElementException; | ||
| import java.util.Objects; |
There was a problem hiding this comment.
This import (java.util.Objects) violates the checklist: "Don't use Objects, Arrays, or any other util class." Do not use Objects.equals(); implement equality checks manually (handle nulls explicitly).
| } | ||
|
|
||
| @Override | ||
| public void add(T value, int index) { |
There was a problem hiding this comment.
The method signature here is add(T value, int index), but the task explicitly refers to add(int index, T value) (the indexed-add should follow the required order). Ensure the method signature matches the requirement/interface and the documented API.
| if (index > size || index < START_INDEX) { | ||
| throw new ArrayListIndexOutOfBoundsException("index %s out of bounds for length %s" | ||
| .formatted(index, size)); | ||
| } |
There was a problem hiding this comment.
You perform index validation here manually instead of reusing the index-check helper. The requirements state you should move repeated index checks to a private helper and use it for add(int index, T value) as well. Also keep in mind that adding at index == size is allowed (the checker must allow upper bound == size).
| for (T elementDatum : elementData) { | ||
| if (Objects.equals(elementDatum, element)) { | ||
| T value = elementData[index]; | ||
| System.arraycopy(elementData, index + 1, elementData, index, size - index); | ||
| size--; |
There was a problem hiding this comment.
The loop iterates over the whole backing array (elementData) including unused slots and uses Objects.equals. Per checklist you must not use Objects and you should iterate only up to size. Also the subsequent System.arraycopy call uses the wrong length: size - index is incorrect for shifting elements after removal — it should be size - index - 1 (same logic as in remove(int index)).
| } | ||
| index++; | ||
| } | ||
| throw new NoSuchElementException(); |
There was a problem hiding this comment.
Throwing NoSuchElementException without an informative message violates the checklist: "Make your exceptions informative." Add a clear message indicating the element was not found (for easier debugging).
| private void grow() { | ||
| capacity += capacity / 2; | ||
| elementData = Arrays.copyOf(elementData, capacity); |
There was a problem hiding this comment.
The grow() method uses Arrays.copyOf(...), which both uses a forbidden util class and does not follow the requirement to allocate a new array and copy elements with System.arraycopy(). Implement grow by creating a new Object[newCapacity] and copying elementData into it with System.arraycopy().
| public String toString() { | ||
| return Arrays.toString(elementData); |
There was a problem hiding this comment.
toString() uses Arrays.toString(elementData), which (1) uses a forbidden util class and (2) prints the whole backing array including nulls beyond size. The result should match java.util.ArrayList and represent only the elements up to size. Implement a manual toString that iterates to size and builds the string.
| @@ -0,0 +1,22 @@ | |||
| package core.basesyntax; | |||
|
|
|||
| public class Main { | |||
There was a problem hiding this comment.
This import violates the checklist: "Don't use Objects, Arrays, or any other util class." You also use Arrays.copyOf and Arrays.toString below; remove this import and avoid java.util.Arrays usage in favor of allocating a new backing array and using System.arraycopy (and building toString using elements up to size).
|
|
||
| public class Main { | ||
| public static void main(String[] args) { | ||
| ArrayList<String> test = new ArrayList<>(); |
There was a problem hiding this comment.
This import violates the checklist: "Don't use Objects, Arrays, or any other util class." Replace Objects.equals usage with an explicit null-safe comparison (e.g. (e == null ? element == null : e.equals(element))).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing, but there are a few important points to fix next time you revisit this task. You did a solid job with the core mechanics: default capacity 10, grow() using a 1.5x factor with System.arraycopy, custom ArrayListIndexOutOfBoundsException, and a correct remove(T value) implementation with null‑safe comparison and NoSuchElementException when not found. The first critical issue is your indexed add method: it currently has the signature add(T value, int index) in ArrayList.java, but it must be add(int index, T value) to match the List interface; when you fix it, also reuse a shared index-check helper that allows index == size for insertion at the end instead of duplicating validation logic. The second critical issue is in remove(int index): after you shift elements left and decrement size, you must set elementData[size] = null; to clear the old last slot, prevent memory leaks, and avoid stale values showing up in iteration or debugging output. A third missing piece is toString(): you need to implement it manually using something like StringBuilder, iterating only from index 0 to size - 1 and formatting like java.util.ArrayList (e.g., [a, b, c]), without using Arrays.toString or printing unused null entries. As a minor improvement, consider extracting the 1.5 growth factor into a named constant instead of using the literal capacity += capacity / 2, and try to centralize index checks to avoid repetition. Overall, your implementation shows good understanding of dynamic arrays and custom collections; tightening these remaining details will bring it fully in line with the usual ArrayList contract and the checklist expectations.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| } | ||
|
|
||
| @Override | ||
| public void add(T value, int index) { |
There was a problem hiding this comment.
The indexed add method uses the signature add(T value, int index). This violates the requirement: "The indexed add method must: Use the signature add(int index, T value) (matching the List interface)." Change the method signature to add(int index, T value) so it matches the List interface and expected API.
| if (index > size || index < START_INDEX) { | ||
| throw new ArrayListIndexOutOfBoundsException("index %s out of bounds for length %s" | ||
| .formatted(index, size)); | ||
| } |
There was a problem hiding this comment.
This block performs its own index validation instead of reusing the shared index-check helper. The requirements state: "Index checking logic that is shared by methods like get(), set(), and remove() must be moved to a separate private method to avoid repeating code" and "The indexed add method must: Use a shared index-check helper method instead of its own index validation." Refactor so the indexed add calls a common helper (or a helper variant) that allows index == size for insertion at the end.
| System.arraycopy(elementData, index + 1, elementData, index, size - index - 1); | ||
| size--; |
There was a problem hiding this comment.
After shifting elements and decrementing size you must null out the former last element position to avoid memory leaks and ensure toString()/iteration don't see stale data. The requirements say: "When an element is removed, shift size - index - 1 elements to close the gap using System.arraycopy. Decrement size. Set the former last element position to null." Add elementData[size] = null; after size--.
| .formatted(index, size)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There is no toString() implementation. The checklist requires: "toString() implementation must: Not use Arrays.toString. Iterate only over indices 0 to size - 1. Build the string manually (for example, using StringBuilder) so output matches java.util.ArrayList and does not print unused null entries." Implement toString() accordingly so output matches java.util.ArrayList and doesn't include unused null slots.
No description provided.