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 replacing Array.prototype.sort with your custom implementation is not yet fulfilled, which means callers of arr.sort() still use the native behavior instead of your sort2 logic.
The critical issue is in src/arrayMethodSort.js: you define [].__proto__.sort2, but you never assign Array.prototype.sort (or [].__proto__.sort) to delegate to sort2, so the built-in sort is not actually overridden as the task requires; you should add a line after the sort2 function that makes Array.prototype.sort call or reference sort2 so that arr.sort(compareFn) uses your implementation. Once that is wired up, the main functional requirement will be met. As a minor improvement, your basicCompare function has somewhat unconventional mixed-case behavior; consider simplifying it (for example, by consistently comparing String(a) and String(b) in a clearly documented way) so the ordering is predictable for all value types. Another non-blocking point is that your bubble-style adjacent-swap algorithm is acceptable for this exercise, but in real projects you might want to mention or switch to a more efficient sort for large arrays.
Overall, you’re very close: the custom sorter itself looks good for the purposes of this task, you just need to hook it up to Array.prototype.sort and consider refining the comparator and comments to make the behavior clearer.
✨ 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.
The task requires that you implement Array.prototype.sort using the predefined [].__proto__.sort2. This file defines sort2 but never assigns/implements Array.prototype.sort. Add something like Array.prototype.sort = [].__proto__.sort2; or Array.prototype.sort = function(compare) { return this.sort2(compare); }; after the sort2 definition so the built-in sort is replaced by your implementation.
| [].__proto__.sort2 = function(compareFunction) { | ||
| // write code here | ||
| [].__proto__.sort2 = function (compareFunction = basicCompare) { | ||
| for (let i = 0; i < this.length - 1; i++) { |
There was a problem hiding this comment.
This loop implements a simple bubble/adjacent-swap sort by restarting the iteration (i = -1) when a swap occurs. Using bubble sort is acceptable for this task, but consider recommending (or switching to) a more efficient algorithm (e.g., quicksort or mergesort) for larger arrays in real-world code.
| if (aLower !== String(a) && bLower === String(b)) { | ||
| return -1; | ||
| } | ||
|
|
||
| if (bLower !== String(b) && aLower === String(a)) { | ||
| return 1; | ||
| } | ||
|
|
||
| return String(a).localeCompare(String(b)); |
There was a problem hiding this comment.
The basicCompare function tries to prefer values with uppercase letters by using aLower !== String(a) checks. This is a bit unconventional and may behave unexpectedly for non-string items. If the goal is case-insensitive comparison, consider simply using String(a).localeCompare(String(b), undefined, { sensitivity: 'base' }) or document the intended tie-breaking behavior clearly.
No description provided.