Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes to your sort2 implementation. While you've done a great job with the basic structure and default string-based sorting, there are a couple of critical issues that prevent it from correctly replicating the native Array.prototype.sort behavior.
Here are the two main points to address:
- Incorrect
compareFunctionarguments: YourcompareFunctionis called with stringified versions of the elements, not the original values. This causes incorrect sorting for non-string data, like numbers. For example,[10, 2].sort2((a, b) => a - b)would not sort correctly. The comparator should receive the original elements to work as expected. - Sparse array support: The use of
this.mapwill cause your function to throw an error when used on sparse arrays (e.g.,[1, , 3]). The nativesortmethod handles these cases gracefully. Please adjust your implementation to correctly handle arrays with empty slots.
Fixing these will ensure your method is a true replication of the native sort. Keep up the great work!
✨ 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
| const records = this.map((item, i) => { | ||
| return { val: item, key: String(item), ind: i }; | ||
| }); |
There was a problem hiding this comment.
Using this.map does not correctly handle sparse arrays (arrays with empty slots). When a sparse array is provided, your code will throw an error because map creates corresponding empty slots in the records array, and later access like records[j].key will fail on these empty slots.
src/arrayMethodSort.js
Outdated
|
|
||
| for (let i = 0; i < records.length; i++) { | ||
| for (let j = 0; j < records.length - 1 - i; j++) { | ||
| const result = sortFunction(records[j].key, records[j + 1].key); |
There was a problem hiding this comment.
The custom compareFunction should receive the original array elements to work correctly for different data types like numbers or objects. Currently, you are always passing the stringified key property, which does not match the behavior of the native sort method and will lead to incorrect results when a comparator is provided.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the previous feedback! Your sort2 method is much closer to the native implementation, especially with how it now handles the compareFunction and avoids crashing on sparse arrays.
However, one final change is needed to fully align with the native sort behavior. Your implementation currently treats empty slots in a sparse array as undefined and passes them to the comparator. The correct behavior is to move all empty slots to the end of the array without ever calling the comparator on them.
Please adjust your algorithm to detect these empty slots and handle them accordingly. Once that's fixed, your solution will be a solid replica!
✨ 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
| for (let i = 0; i < this.length; i++) { | ||
| for (let j = 0; j < this.length - 1 - i; j++) { | ||
| const result = sortFunction(this[j], this[j + 1]); | ||
|
|
||
| if (result > 0) { | ||
| [this[j], this[j + 1]] = [this[j + 1], this[j]]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This sorting logic doesn't correctly handle sparse arrays (e.g., [1, , 3]). The native sort method should move empty slots to the end of the array without passing their undefined value to the compareFunction.
Your current implementation will pass undefined to the comparator, which can lead to incorrect results, especially with a numeric comparison function (e.g., (a, b) => a - b would result in NaN).
Please update the loop to detect and correctly handle these empty slots.
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! I am approving your solution as you've successfully addressed the feedback on handling sparse arrays. Your implementation now correctly moves all empty slots to the end of the array without passing them to the comparator function, perfectly mimicking the native sort behavior. Your solution is complete and meets all the task requirements. 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
ready for code review