Conversation
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 your own version of the sort method! The core logic is solid. However, I am requesting a change because the function's handling of sparse arrays (arrays with empty slots) differs from the native Array.prototype.sort behavior. Your implementation currently treats empty slots as undefined and sorts them, while the native method moves all empty slots to the end of the array. Please adjust your logic to correctly handle this case. Also, please fix the small typo in the compair variable name to compare. You're very close to a complete solution!
✨ 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
| // write code here | ||
| [].__proto__.sort2 = function (compareFunction) { | ||
| // const newArray = []; | ||
| let compair = compareFunction; |
There was a problem hiding this comment.
You have a small typo in the variable name here. It should be compare instead of compair. This typo is used consistently throughout the function, so be sure to correct it in all places.
| String(a) > String(b) ? 1 : String(a) < String(b) ? -1 : 0; | ||
| } | ||
|
|
||
| for (let i = 0; i < this.length - 1; i++) { |
There was a problem hiding this comment.
This implementation doesn't handle sparse arrays (arrays with empty slots) in the same way as the native Array.prototype.sort. The native method moves all empty slots to the end of the array, whereas your current logic treats them as undefined and includes them in the sorting comparison based on the string 'undefined'.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thanks for fixing the typo from the last review!
While the sorting logic works for standard arrays, this implementation requires changes because it doesn't correctly handle sparse arrays. The native Array.prototype.sort moves all empty slots to the end of the array, but your current logic leaves them in their original relative positions.
The new block of code you added to reconstruct the array has incorrect loop conditions (e.g., i < this.length - 1), which causes it to skip the last element. A more effective approach is to handle empty slots directly within your main sorting loop. Consider modifying your comparison logic to treat empty slots as being 'greater' than any defined value, which will cause them to be sorted to the end naturally.
✨ 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
| if (this[i] === undefined) { | ||
| continue; | ||
| } | ||
|
|
||
| if (this[j] === undefined) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This logic for handling undefined values doesn't correctly implement the behavior for sparse arrays. The native sort method moves all empty slots (which are different from undefined values) to the end. Simply skipping these elements leaves them in their relative positions instead of moving them. Consider modifying your comparison logic to treat empty slots as being 'greater' than any defined value, which will cause them to be sorted to the end.
src/arrayMethodSort.js
Outdated
| for (let i = 0; i < this.length - 1; i++) { | ||
| if (this[i] !== undefined) { | ||
| result[result.length] = this[i]; | ||
| } | ||
| } | ||
|
|
||
| for (let i = 0; i < result.length - 1; i++) { | ||
| this[i] = result[i]; | ||
| } |
There was a problem hiding this comment.
This block of code intended to reconstruct the array has a couple of issues. The loop conditions, such as i < this.length - 1, will cause the last element of the array to be skipped in both loops. A more direct approach would be to handle the empty slots within the main sorting loop, which would make this extra reconstruction step unnecessary.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission as an educational override to help you move forward, as there's one key detail left to perfect. You've done an excellent job addressing the previous feedback, especially with moving empty values to the end and fixing the loop conditions!
The main remaining issue is that your implementation treats empty slots in a sparse array the same as an explicit undefined value, converting the empty slots to undefined. The native sort method preserves these empty slots. To fix this, you can distinguish between these two cases by checking if an index actually exists in the array using the in operator (e.g., i in this).
Fantastic progress on a tricky problem. You're very close to a complete solution!
✨ 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++) { | ||
| if (this[i] !== undefined) { | ||
| result[result.length] = this[i]; | ||
| } else { | ||
| undefinedNumber[undefinedNumber.length] = this[i]; | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code correctly separates defined values from undefined/empty ones. However, it treats an empty slot in a sparse array the same as an element that is explicitly undefined. This causes empty slots to be converted to undefined values in the final sorted array, which differs from the native sort behavior.
To fix this, you need to distinguish between these two cases. You can check if an index actually exists in the array using the in operator (e.g., i in this). This will be true for an index with an undefined value but false for an empty slot.
2 participants
No description provided.