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 several critical checklist and requirement violations that would currently block this task from being considered complete. The main blockers are: (1) use of java.util.Arrays and Arrays.copyOf(...) in src/main/java/core/basesyntax/ArrayList.java, which is explicitly disallowed by the task (“Don’t use Objects, Arrays, or any other util class”) — instead, introduce a dedicated resize/ensureCapacity method that creates a new array and uses System.arraycopy() to copy elements. (2) The custom ArrayListIndexOutOfBoundsException is thrown in your index checks but the class itself is missing; you need to add core.basesyntax.ArrayListIndexOutOfBoundsException extending IndexOutOfBoundsException (or RuntimeException) with informative constructors/messages so the project compiles and matches the description. (3) Element shifting in add(index, ...) and remove(int index) is done via manual loops, while the checklist requires using System.arraycopy() to move array elements; replace those loops with appropriate System.arraycopy() calls for shifting right/left. Additionally, some magic numeric literals in growth logic (the / 2 for 1.5× growth and the + 1 fallback) should be extracted into private static final constants, and addAll() should reuse the same resize/ensureCapacity method instead of performing inline array reallocation.
Once you remove Arrays usage, add the missing exception class, switch all shifts to System.arraycopy(), and centralize capacity management into a helper method with named constants for magic numbers, your implementation will align well with both the functional and checklist requirements. You already have solid core behavior (default capacity, 1.5× growth idea, clear index-check helpers, and informative messages), so these changes are mostly about meeting the strict constraints of this exercise. Keep up the good work and treat these adjustments as polishing a nearly complete solution.
✨ 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,123 @@ | |||
| package core.basesyntax; | |||
|
|
|||
| import java.util.Arrays; | |||
There was a problem hiding this comment.
This import brings in java.util.Arrays which is disallowed by the checklist: "Don't use Objects, Arrays, or any other util class." Remove this import and implement resizing using System.arraycopy() in your own resize method.
| if (newArrayLength == elementData.length) { | ||
| newArrayLength = elementData.length + 1; | ||
| } | ||
| elementData = Arrays.copyOf(elementData, newArrayLength); |
There was a problem hiding this comment.
Using Arrays.copyOf(...) here violates: "Don't use Objects, Arrays, or any other util class." Implement resizing via a dedicated resize/ensureCapacity method that uses System.arraycopy() and then assign the new array to elementData.
| } | ||
|
|
||
| private void grow() { | ||
| int newArrayLength = elementData.length + (elementData.length / 2); |
There was a problem hiding this comment.
This growth calculation embeds a magic number via / 2 (the 1.5x behavior). The checklist requires: "Any magic numbers should be constants" — extract the growth factor expression or the divisor into a descriptive private static final constant so the intent is explicit.
| private void grow() { | ||
| int newArrayLength = elementData.length + (elementData.length / 2); | ||
| if (newArrayLength == elementData.length) { | ||
| newArrayLength = elementData.length + 1; |
There was a problem hiding this comment.
This fallback + 1 is a magic literal; it violates: "Any magic numbers should be constants". Introduce a named constant (e.g. MIN_GROW_STEP) instead of an inline 1.
| for (int i = size; i > index; i--) { | ||
| elementData[i] = elementData[i - 1]; | ||
| } |
There was a problem hiding this comment.
You shift elements with a manual for-loop when inserting at index. The checklist asks: "Use [System.arraycopy()] to move your array elements." Replace this loop with a System.arraycopy() call to move the tail elements.
| for (int i = index; i < size - 1; i++) { | ||
| elementData[i] = elementData[i + 1]; | ||
| } |
There was a problem hiding this comment.
Removal shifts elements using a manual loop; replace it with System.arraycopy() as required by the checklist: "Use [System.arraycopy()] to move your array elements." This will also reduce duplication and improve clarity.
| T[] newArray = (T[]) new Object[newSize]; | ||
| System.arraycopy(elementData, 0, newArray, 0, size); | ||
| elementData = newArray; |
There was a problem hiding this comment.
In addAll() you create a new array inline and assign it to elementData; the checklist requires: "Resize the array in a separate method." Move resizing/ensure-capacity logic into a dedicated method (reuse the same method that add/add(index) use) and use it here.
|
|
||
| private void checkIndex(int index) { | ||
| if (index < 0 || index >= size) { | ||
| throw new ArrayListIndexOutOfBoundsException("Index: " + index + " Size: " + size); |
There was a problem hiding this comment.
The code throws ArrayListIndexOutOfBoundsException in index checks, but there is no custom exception class present in the project. The task description explicitly requires: "Throw custom ArrayListIndexOutOfBoundsException in case the index passed to any of the methods is invalid." Add the custom exception class (with informative constructors/messages) to the package so the code compiles and meets the requirement.
|
|
||
| private void checkIndexForAdd(int index) { | ||
| if (index < 0 || index > size) { | ||
| throw new ArrayListIndexOutOfBoundsException("Index: " + index + " Size: " + size); |
There was a problem hiding this comment.
Same as previous: checkIndexForAdd throws ArrayListIndexOutOfBoundsException but the custom exception class is missing. Implement and include ArrayListIndexOutOfBoundsException (with informative message) per task description.
| int newSize = size + list.size(); | ||
| if (newSize > elementData.length) { | ||
| T[] newArray = (T[]) new Object[newSize]; | ||
| System.arraycopy(elementData, 0, newArray, 0, size); |
There was a problem hiding this comment.
addAll() already uses System.arraycopy() for copying into the new array (good), but overall the class is inconsistent: other element moves still use loops. Ensure all shifting/copying uses System.arraycopy() and centralize capacity logic into a single resize/ensureCapacity method to avoid repetition and satisfy multiple checklist items (DRY, use of System.arraycopy(), resize in separate method).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
4 participants
No description provided.