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 fully met yet: you implemented Array.prototype.sort2 correctly, but you did not override Array.prototype.sort to delegate to sort2, which means tests expecting the custom sort implementation will still fail. In src/arrayMethodSort.js, inside applyCustomSort, after defining sort2 and before returning, you should assign something like Array.prototype.sort = function(compareFn) { return this.sort2(compareFn); }; so that all array sort calls use your custom logic. This missing override is a functional blocker, since the main task is specifically to implement Array.prototype.sort via the provided sort2 hook.
The sort2 implementation itself is solid: bubble sort is acceptable here, your swapping logic is fine, and returning this correctly supports chaining. As a minor (non-blocking) improvement, you could add a type check that throws a TypeError if compareFn is provided but is not a function, to better mirror native behavior. Once you add the Array.prototype.sort delegation, your solution should satisfy the primary requirements, and the rest of the code looks good for this exercise.
✨ 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.
Before returning from applyCustomSort, assign Array.prototype.sort to delegate to your sort2 implementation. For example:
Array.prototype.sort = function(compareFn) {
return this.sort2(compareFn);
};This is required by the task description (implement Array.prototype.sort using the predefined sort2).
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 is not overridden to delegate to your custom sort2 implementation. As a result, calls like someArray.sort() will still use the native engine implementation instead of your bubble-sort-based logic, which means the main functionality described in the task is not actually in effect. To fix this, immediately after your sort2 definition in src/arrayMethodSort.js, add an override such as:
Array.prototype.sort = function (compareFn) {
return this.sort2(compareFn);
};This will ensure all standard sort calls use your custom algorithm and will satisfy the functional requirement. As an optional improvement (not a blocker), you can add a check that if compareFn is provided and is not a function, you throw a TypeError, which will make your behavior closer to the native Array.prototype.sort. Your bubble sort implementation, element swapping, and returning this are solid for this exercise—once the override is added, the solution should align well with the task expectations.
✨ 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.
Missing override of Array.prototype.sort to delegate to your sort2. Add something like Array.prototype.sort = function(compareFn) { return this.sort2(compareFn); }; immediately after the sort2 definition so array.sort() uses the custom implementation (this is required by the task description).
| function applyCustomSort() { | ||
| [].__proto__.sort2 = function(compareFunction) { | ||
| // write code here | ||
| [].__proto__.sort2 = function (compareFunction) { |
There was a problem hiding this comment.
Optional: validate compareFunction type. If compareFunction is provided but is not a function, consider throwing a TypeError to better mirror native Array.prototype.sort behavior (not strictly required by tests but improves compatibility).
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
There was a problem hiding this comment.
Review Summary
Your solution is approved! You've done a fantastic job addressing the previous blocking issues and successfully implementing the Array.prototype.sort override to delegate to your custom sort2 function. This is a significant step forward in understanding prototype methods and custom implementations. While I am approving this for educational progress, please note there's a minor improvement you could consider for even greater robustness. As an optional enhancement, I recommend adding a check within your sort2 implementation to ensure that if a compareFunction is provided, it is indeed a function. If it's not a function, throwing a TypeError would align your custom sort's behavior more closely with the native Array.prototype.sort. Keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const cmp = | ||
| compareFunction || |
There was a problem hiding this comment.
As an optional improvement, you could add a check here to ensure that if compareFunction is provided, it is actually a function. If compareFunction is provided but is not a function, the native Array.prototype.sort throws a TypeError. Implementing this would make your custom sort's behavior even closer to the native one.
| for (let i = 0; i < this.length - 1; i++) { | ||
| for (let j = 0; j < this.length - i - 1; j++) { | ||
| if (cmp(this[j], this[j + 1]) > 0) { | ||
| const temp = this[j]; | ||
|
|
||
| this[j] = this[j + 1]; | ||
| this[j + 1] = temp; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This bubble sort implementation works correctly. For future reference, if performance were a critical concern, more efficient sorting algorithms like QuickSort or MergeSort would be preferred, as bubble sort has a time complexity of O(n^2). However, for this task, it's perfectly acceptable.
No description provided.