did everything you asked for#1741
did everything you asked for#1741OleksandrYurkin wants to merge 3 commits intomate-academy:masterfrom
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is a good start on your custom ArrayList, but it requires changes because several key requirements from the task checklist have not been met. Specifically, your code has repeating logic for resizing the array and for checking index bounds; this logic must be extracted into separate private helper methods [CHECKLIST ITEM #5, #9]. Additionally, the task requires using System.arraycopy() for shifting array elements, but your implementation uses for loops [CHECKLIST ITEM #8]. Please also define the growth factor 1.5 as a constant [CHECKLIST ITEM #3] and add an informative message to the NoSuchElementException [CHECKLIST ITEM #10]. Addressing these points will bring your solution in line with the requirements.
✨ 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
| if (size == objects.length) { | ||
| Object[] copy = new Object[(int) (objects.length * 1.5)]; | ||
| System.arraycopy(objects, 0, copy, 0, objects.length); | ||
| objects = copy; | ||
| } |
There was a problem hiding this comment.
| public void add(T value) { | ||
|
|
||
| if (size == objects.length) { | ||
| Object[] copy = new Object[(int) (objects.length * 1.5)]; |
There was a problem hiding this comment.
The growth factor 1.5 is a magic number. This violates checklist item #3: 'Any magic numbers should be constants'. Please declare it as a private static final constant with a descriptive name, for example, GROWTH_FACTOR.
| if (index < 0 || index > size) { | ||
| throw new ArrayListIndexOutOfBoundsException("Index out of bound"); |
There was a problem hiding this comment.
The logic for checking the index is repeated in get(), set(), and remove(int). This violates checklist item #5, which states: 'the index checks for the get(), set(), and remove() methods should be moved to a separate method.' Please extract this validation into a private helper method.
| for (int i = size; i > index; i--) { | ||
| objects[i] = objects[i - 1]; | ||
| } |
There was a problem hiding this comment.
This for loop is used to shift elements to make space for the new one. However, this violates checklist item #8: 'Use System.arraycopy() to move your array elements.' You should use System.arraycopy() here for shifting.
| for (int i = index; i < size - 1; i++) { | ||
| objects[i] = objects[i + 1]; | ||
| } |
There was a problem hiding this comment.
This for loop is used to shift elements after a removal. This violates checklist item #8: 'Use System.arraycopy() to move your array elements.' Please use System.arraycopy() to perform this operation.
| for (int j = i; j < size - 1; j++) { | ||
| objects[j] = objects[j + 1]; | ||
| } |
There was a problem hiding this comment.
Similar to the remove(int index) method, this for loop should be replaced with a System.arraycopy() call to adhere to checklist item #8: 'Use System.arraycopy() to move your array elements.'
| return removed; | ||
| } | ||
| } | ||
| throw new java.util.NoSuchElementException(); |
There was a problem hiding this comment.
This exception is thrown without a message. This violates checklist item #10: 'Make your exceptions informative'. Please provide a descriptive message, such as "Element not found in the list".
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You've successfully addressed almost all the points from the previous review, and the code is much more robust. I am requesting one small change before approving.
In your ensureCapacity() method within ArrayList.java, the line int newCapacity = (int) (objects.length * GROWTH_FACTOR) + 1; contains a magic number (1). According to the project requirements, all magic numbers should be moved to constant fields with informative names [CHECKLIST ITEM #3].
Please extract this value into a constant, for example, private static final int MINIMUM_GROWTH_AMOUNT = 1;. This will make the code more readable and maintainable. Once you've made this change, the solution will be perfect!
✨ 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 ensureCapacity() { | ||
| if (size == objects.length) { | ||
| int newCapacity = (int) (objects.length * GROWTH_FACTOR) + 1; |
There was a problem hiding this comment.
This line uses a magic number 1. According to checklist item #3, all magic numbers should be moved to constant fields with informative names. This will improve code readability and maintainability. For example, you could create a constant like private static final int MINIMUM_GROWTH_AMOUNT = 1;.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work! You've successfully addressed the feedback from the previous review by extracting the magic number into a constant. The code is now clean, easy to read, and fully compliant with all task requirements. I'm happy to approve your solution. Well done!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.