Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the core requirement of the task is not yet met: Array.prototype.sort is never implemented using the predefined [].__proto__.sort2, so any code calling Array.prototype.sort will still use the native implementation instead of your custom one. The main critical fix is to, after defining sort2, explicitly delegate Array.prototype.sort to it, for example by assigning Array.prototype.sort = [].__proto__.sort2; or by wrapping a call to sort2 so that it operates on this.
The other issues are less critical but still worth addressing: when a compareFunction is provided but is not actually a function, the native behavior is to throw a TypeError, whereas your implementation silently falls back to string comparison—consider adding a typeof compareFunction === 'function' check and throwing if it fails to better mirror native behavior. Also, bubble sort is O(n²); this is acceptable for the exercise, but you might want to add a short comment noting the performance characteristics so future readers understand the limitation. Overall, your sort2 implementation itself is on the right track (in-place, returns the array, supports a comparator), and with the delegation to Array.prototype.sort plus the small behavioral tweak, your solution will align well with the task 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
| function applyCustomSort() { | ||
| [].__proto__.sort2 = function(compareFunction) { | ||
| // write code here | ||
| [].__proto__.sort2 = function (compareFunction) { |
There was a problem hiding this comment.
This creates [].__proto__.sort2, but the task requires implementing Array.prototype.sort using that predefined sort2. The file never assigns or implements Array.prototype.sort. Consider adding a line such as Array.prototype.sort = [].__proto__.sort2; (or similar) after defining sort2 so the built-in method is implemented.
| if (typeof compareFunction === 'function') { | ||
| shouldSwap = compareFunction(this[o], this[o + 1]) > 0; | ||
| } else { | ||
| shouldSwap = String(this[o]) > String(this[o + 1]); | ||
| } |
There was a problem hiding this comment.
If a compareFunction is provided but is not a function, the native Array.prototype.sort throws a TypeError. Here the code silently falls back to string comparison. Consider validating compareFunction and throwing TypeError when it's not a function to match native behavior (or explicitly document the difference).
| for (let i = 0; i < this.length; i++) { | ||
| for (let o = 0; o < this.length - 1 - i; o++) { |
There was a problem hiding this comment.
Using a simple bubble sort here is allowed for the task, but it's O(n²) and will perform poorly on large arrays. Consider mentioning this limitation or replacing it with a more efficient algorithm (quicksort/mergesort) if performance matters.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a core requirement of the task is not yet met: Array.prototype.sort still does not use your custom sort2 implementation. Right now you define [].__proto__.sort2 in src/arrayMethodSort.js, but you never actually redirect Array.prototype.sort to call it, so someArray.sort(...) continues to use the native sort instead of your implementation. To fix this, immediately after the line that ends the sort2 assignment (around line 34, right after the closing };), add a delegation such as:
Array.prototype.sort = [].__proto__.sort2;so that all arr.sort(...) calls run through your custom logic. As a minor, non-blocking improvement, add a short comment near the nested loops noting that this is a bubble sort with O(n²) time complexity, so future readers understand the performance trade-off. Your comparator validation and in-place swapping look good; once the delegation is in place, your solution will satisfy the functional 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
| } | ||
| } | ||
|
|
||
| return this; |
There was a problem hiding this comment.
You define [].__proto__.sort2 but never make Array.prototype.sort delegate to it. Add a delegation (for example, after the sort2 assignment) such as:
Array.prototype.sort = [].__proto__.sort2;
so calls to someArray.sort(...) use your custom implementation. Place that new line after the }; that ends the sort2 assignment (i.e., immediately after line 34).
| for (let i = 0; i < this.length; i++) { | ||
| for (let o = 0; o < this.length - 1 - i; o++) { |
There was a problem hiding this comment.
This implementation uses bubble sort (the nested loops starting here). That's acceptable for the task, but consider adding a short comment noting the O(n²) performance so future readers understand the limitation and why a more efficient algorithm (e.g., quicksort or mergesort) would be preferable in production.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution — the custom sort2 implementation is correctly wired so that Array.prototype.sort delegates to it, you validate the comparator, perform in-place swaps, and return this for chaining. One improvement opportunity: near the nested loops in src/arrayMethodSort.js (around your double for loops), add a short comment noting that this is a bubble sort with O(n²) time complexity; this helps future readers immediately understand the performance trade-off. This is an educational override to help you progress while learning, since the missing comment is about clarity rather than correctness. Functionally, your approach meets the requirement to base Array.prototype.sort on the predefined [].__proto__.sort2, and you’ve handled the comparator behavior in a reasonable way. As a minor enhancement, you could also add a brief comment above the comparator check explaining that a non-function comparator will throw a TypeError, which documents the intended usage. Overall, the core logic is solid and meets the task requirements—nice work on implementing a complete custom sort and keeping it chainable.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| for (let i = 0; i < this.length; i++) { | ||
| for (let o = 0; o < this.length - 1 - i; o++) { | ||
| let shouldSwap; |
There was a problem hiding this comment.
Consider adding a short comment above these nested loops noting that this is a bubble sort (O(n²)) — it's acceptable for this task, but helps future readers understand the performance trade-off.
No description provided.