Implemented methods for ArrayList class#1793
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your ArrayList implementation has critical checklist violations that must be addressed. The most important issue is that you're using the java.util.Arrays utility class (Arrays.copyOf() and Arrays.toString()), which is explicitly prohibited by [CHECKLIST ITEM #2] — these must be replaced with manual array copying and iteration logic. Additionally, the magic number 10 for default capacity should be extracted to a constant like DEFAULT_CAPACITY per [CHECKLIST ITEM #3]. Finally, the index validation logic (index < size() && index >= 0) is duplicated in get(), set(), and remove() methods — extract this to a private validateIndex(int index) helper method as required by [CHECKLIST ITEM #5]. There's also a logic issue: your add(T value) method currently resizes on every call regardless of whether capacity is needed, which is inefficient — the resize should only occur when size >= capacity. The core functionality is working correctly (growth factor, exception handling, method implementations), but these checklist violations prevent approval. Please fix these issues and resubmit.
✨ 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
| import java.util.NoSuchElementException; | ||
|
|
||
| public class ArrayList<T> implements List<T> { | ||
| private Object[] elementData = new Object[10]; |
There was a problem hiding this comment.
Magic number 10 should be a constant per checklist item #3. Create a constant like DEFAULT_CAPACITY = 10 and use it here.
| int capacityArray = elementData.length; | ||
| if (size() >= elementData.length) { | ||
| capacityArray += (capacityArray >> 1); | ||
| return Arrays.copyOf(elementData, capacityArray); |
There was a problem hiding this comment.
Per checklist item #2: Do not use Arrays utility class. Replace Arrays.copyOf() with manual copying logic.
| } | ||
|
|
||
| @Override | ||
| public T get(int index) { |
There was a problem hiding this comment.
Per checklist item #5: The index validation logic index < size() && index >= 0 is repeated in get(), set(), and remove(int index) methods. Move this to a private helper method like validateIndex(int index).
| } | ||
|
|
||
| @Override | ||
| public void set(T value, int index) { |
There was a problem hiding this comment.
Per checklist item #5: Same index validation repeated here - should use the shared private method.
| public T remove(int index) { | ||
| return null; | ||
| T deletedElement; | ||
| if (index < size() && index >= 0) { |
There was a problem hiding this comment.
Per checklist item #5: Same index validation repeated here - should use the shared private method.
| @Override | ||
| public boolean isEmpty() { |
There was a problem hiding this comment.
Per checklist item #4: This if-else can be simplified to return size() == 0;
| return arrayFromCollection; | ||
| } | ||
|
|
||
| public Object[] getNewCapacityArray(Object[] elementData) { |
There was a problem hiding this comment.
Per checklist item #8: The getNewCapacityArray method both checks if resize is needed AND creates the new array. The growth check is currently done outside the method in add(), causing unnecessary array copying on every add. Consider having the method only handle growth when needed, or ensure the method itself determines when growth is necessary.
| return arrayFromCollection; | ||
| } | ||
|
|
||
| public Object[] getNewCapacityArray(Object[] elementData) { |
There was a problem hiding this comment.
Parameter name shadows the field name elementData. Use a different name like sourceArray.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The core ArrayList implementation is working correctly with proper growth factor (1.5x), correct exception handling, and no usage of forbidden utility classes. All HIGH-priority issues from the previous review have been successfully resolved. However, there are a few remaining improvements to address: 1. Visibility Issue: Change public to private for helper methods checkCapacity(), validateIndex(), getArrayFromCollection(), and getNewCapacityArray(). These are internal implementation details that shouldn't be exposed via the public API. 2. Typo Fix: Rename rigthIndex to rightIndex in the remove(int index) method. 3. Code Simplification: The isEmpty() method can be simplified from if (size == 0) return true; else return false; to return size == 0;. Similarly, validateIndex() can directly return the boolean expression instead of using if-else. These are style and cleanliness improvements rather than functional blockers. The implementation correctly fulfills all task requirements and the tests pass. Great work on fixing the previous critical issues!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| @Override | ||
| public boolean isEmpty() { | ||
| if (size() == 0) { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
This violates checklist item #4: 'Don't complicate if-else construction'. The if-else can be replaced with a simple return statement.
| public boolean validateIndex(int index) { | ||
| if (index < size() && index >= 0) /*&& elementData[index] != null)*/ { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
This violates checklist item #5: The validateIndex() method should throw the exception directly instead of returning boolean, eliminating the if-else pattern in calling methods. Compare to the 'Bad example' in the checklist.
| @Override | ||
| public T get(int index) { | ||
| return null; | ||
| boolean rightIndex = validateIndex(index); | ||
| if (rightIndex == true) { | ||
| return (T) elementData[index]; | ||
| } else { | ||
| throw new ArrayListIndexOutOfBoundsException("This index doesn't exist"); | ||
| } |
There was a problem hiding this comment.
This violates checklist item #5: The get() method has unnecessary if-else after validateIndex(). Same issue applies to set() and remove(int) methods.
| @Override | ||
| public void set(T value, int index) { | ||
|
|
||
| boolean rightIndex = validateIndex(index); | ||
| if (rightIndex == true) { | ||
| elementData[index] = value; | ||
| } else { | ||
| throw new ArrayListIndexOutOfBoundsException("This index doesn't exist"); | ||
| } |
There was a problem hiding this comment.
This violates checklist item #5: The set() method has unnecessary if-else after validateIndex().
| @Override | ||
| public T remove(int index) { | ||
| return null; | ||
| T deletedElement; | ||
| boolean rigthIndex = validateIndex(index); | ||
| if (rigthIndex == true) { | ||
| deletedElement = (T) elementData[index]; | ||
| System.arraycopy(elementData, index + 1, elementData, index, size() - index - 1); | ||
| elementData[size - 1] = null; | ||
| size--; | ||
| return deletedElement; | ||
| } | ||
| throw new ArrayListIndexOutOfBoundsException("This index doesn't exist"); |
There was a problem hiding this comment.
This violates checklist item #5: The remove(int) method has unnecessary if-else after validateIndex().
| return false; | ||
| } | ||
|
|
||
| public Object[] getArrayFromCollection(List<T> list) { |
There was a problem hiding this comment.
This violates checklist item #6: 'Don't use abstract words like array as variable names'. Consider renaming 'arrayFromCollection' to something more informative like 'elementsFromCollection'.
| @Override | ||
| public T remove(T element) { | ||
| return null; | ||
| for (int i = 0; i < size(); i++) { |
There was a problem hiding this comment.
This violates checklist item #6: 'Do not use one-letter names'. The variable 'i' is used in loops. While this is common in Java, consider if more descriptive iteration variable names would help.
| return false; | ||
| } | ||
|
|
||
| public Object[] checkCapacity(Object[] elementData) { |
There was a problem hiding this comment.
This violates checklist item #7: These helper methods (checkCapacity, validateIndex, getArrayFromCollection, getNewCapacityArray) are not part of the List interface and should be private, not public.
| public Object[] checkCapacity(Object[] elementData) { | ||
| if (size() >= elementData.length) { | ||
| elementData = getNewCapacityArray(elementData); | ||
| return elementData; | ||
| } | ||
| return elementData; |
There was a problem hiding this comment.
The checkCapacity method creates redundant assignments. Since the method returns a value either way, you can simply return the result directly without the intermediate variable assignment.
2 participants
No description provided.