Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job setting up the sort2 method on the Array prototype! However, this requires a change to meet the core requirements. The main issue is that your default compareFunction does not correctly replicate the native sort() behavior. It returns a boolean, but a comparison function must return a number (negative, zero, or positive) to indicate the sort order. This causes arrays like [10, 1] to be sorted incorrectly. To fix this, the default function should compare elements as strings and return a numeric value. You might find String.prototype.localeCompare() helpful here. Once you've updated the comparison logic, I'll be happy to take another look!
✨ 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
src/arrayMethodSort.js
Outdated
| [].__proto__.sort2 = function(compareFunction) { | ||
| // write code here | ||
| [].__proto__.sort2 = function ( | ||
| compareFunction = (a, b) => String(a) > String(b), |
There was a problem hiding this comment.
The default compareFunction is not correctly implemented. It should return a number (negative, 0, or positive) to be compliant with the Array.prototype.sort specification, but it returns a boolean. For example, sorting [10, 1] will not work as expected because '10' > '1' evaluates to false. A correct implementation could use String.prototype.localeCompare or explicit checks to return -1, 0, or 1.
src/arrayMethodSort.js
Outdated
| compareFunction = (a, b) => String(a) > String(b), | ||
| ) { | ||
| for (let i = 0; i < this.length - 1; i++) { | ||
| for (let j = 0; j < this.length - 1; j++) { |
There was a problem hiding this comment.
This bubble sort implementation can be slightly optimized. After each full pass of the outer loop, the largest element for that pass is moved to its correct sorted position at the end of the unsorted portion of the array. Therefore, the inner loop doesn't need to check the elements that are already sorted. You can adjust the inner loop's condition to j < this.length - 1 - i.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the custom sort method! You're very close to a complete solution. The main issue is that your default compareFunction returns a boolean (true/false) instead of a number.
The native Array.prototype.sort specification requires the comparator to return a negative number, zero, or a positive number to determine the sort order. While your current logic works due to type coercion, it doesn't correctly replicate the required behavior.
Please update the function to return a numeric value. For the default string comparison, String(a).localeCompare(String(b)) is an excellent way to achieve this. After that one change, this 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
src/arrayMethodSort.js
Outdated
| let cmp = compareFunction; | ||
|
|
||
| if (typeof cmp !== 'function') { | ||
| cmp = (a, b) => `${a}` > `${b}`; |
There was a problem hiding this comment.
Great progress on fixing the sorting logic! However, the compareFunction is still returning a boolean (true/false) instead of a number as required by the sort method's specification. A proper compare function must return a positive number, a negative number, or zero. While this works in your if statement due to type coercion, it doesn't fully replicate the native sort behavior. Please adjust this to return a numeric value, for example by using String.prototype.localeCompare().
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 task! I am approving your solution. You've done a fantastic job of addressing the previous feedback by updating the default compareFunction to return a numeric value (-1, 0, or 1). This change correctly aligns your implementation with the behavior of the native sort method and fulfills all the requirements. As a suggestion for your continued learning, you could explore more efficient sorting algorithms like Merge Sort or Quick Sort. For this task, however, your implementation is perfect. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.