Maksym Gorodnychyi#1784
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has four critical violations of task requirements that must be fixed. First, the array growth factor is 2x instead of the required 1.5x multiplier (change elements.length * 2 to calculate 1.5x growth). Second, you're throwing IndexOutOfBoundsException instead of ArrayListIndexOutOfBoundsException in checkIndex() and other methods. Third, your remove(T element) method returns null when the element isn't found, but it should throw NoSuchElementException. Fourth, you must use System.arraycopy() for copying array elements in growArray() and add(T value, int index) methods instead of manual for-loops, as specified in the checklist requirements. Additionally, there are checkstyle violations at lines 33 (method ordering) and 79 (variable declaration distance) that need addressing. These are explicit functional requirements that the current implementation doesn't meet.
✨ 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 void growArray() { | ||
| T[] newElements = (T[]) new Object[elements.length * 2]; |
There was a problem hiding this comment.
Array grows at 2x (elements.length * 2), but the task requires 1.5x growth factor. Change this to use 1.5x multiplication.
| if (index < 0 || index > size) { | ||
| throw new IndexOutOfBoundsException("Index: " + index + ", Size: " + size); |
There was a problem hiding this comment.
Should throw ArrayListIndexOutOfBoundsException instead of IndexOutOfBoundsException to match task requirements.
| private void checkIndex(int index) { | ||
| if (index < 0 || index >= size) { |
There was a problem hiding this comment.
Should throw ArrayListIndexOutOfBoundsException instead of IndexOutOfBoundsException to match task requirements.
| } | ||
| return null; |
There was a problem hiding this comment.
The task requires throwing NoSuchElementException when element is not found, but the method returns null instead.
| for (int i = 0; i < size; i++) { | ||
| newElements[i] = elements[i]; |
There was a problem hiding this comment.
Checklist requires using System.arraycopy() to move array elements, not a manual for-loop.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your ArrayList implementation is approved. All core functional requirements are working correctly: the 1.5x growth factor is properly calculated using elements.length + elements.length / 2, the correct exception types are thrown (ArrayListIndexOutOfBoundsException and NoSuchElementException), System.arraycopy() is used appropriately in array resizing operations, and constants are properly defined. There is one minor style issue to be aware of: there's a redundant empty line inside the for-loop body at line 84. While this doesn't affect functionality, it's a good practice to keep loop bodies compact without unnecessary blank lines between the opening brace and the first statement. This style issue is minor and won't block approval, but cleaning it up would improve code readability. Great work on getting all the core logic correct!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| public T remove(T element) { | ||
| return null; | ||
| for (int i = 0; i < size; i++) { | ||
|
|
There was a problem hiding this comment.
Remove the redundant empty line inside the for-loop body. The loop iteration code should follow directly after the for statement.
fetis-off
left a comment
fetis-off
left a comment
There was a problem hiding this comment.
Good work. Minor: private methods should be after public methods
3 participants
No description provided.