Skip to content

Implement custom sort method#2501

Open
CyborgNinjaHat wants to merge 3 commits intomate-academy:masterfrom
CyborgNinjaHat:develop
Open

Implement custom sort method#2501
CyborgNinjaHat wants to merge 3 commits intomate-academy:masterfrom
CyborgNinjaHat:develop

Conversation

@CyborgNinjaHat
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Changes are needed because a core requirement of the task is not fully met: your custom implementation is only attached as [].__proto__.sort2 and does not actually hook into Array.prototype.sort, so any code that calls arr.sort(...) will still use the native implementation instead of yours. In src/arrayMethodSort.js, after you define [].__proto__.sort2 = function sort2(...) { ... }, you should also delegate Array.prototype.sort to this implementation, for example by assigning Array.prototype.sort = Array.prototype.sort2 or by implementing Array.prototype.sort to call this.sort2(...) [CHECKLIST ITEM #1, #8]. This wiring is essential for the tests and consumers that expect your custom logic to run when sort is called, so without it the task is considered incomplete.

Aside from that, your bubble-sort implementation and destructuring swap look correct and aligned with the requirements, which is great. For robustness, consider adding an explicit type check at the start of sort2 so that if compareFunction is provided and is not a function you throw a TypeError, matching native Array.prototype.sort behavior. You might also note that bubble sort is O(n²); it’s acceptable for this assignment, but in real-world scenarios you would typically use a more efficient algorithm or delegate to the built-in sort. Once you wire up Array.prototype.sort to use your sort2 implementation and, ideally, add the comparator type check, your solution will satisfy the core requirements and be in very good shape.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/arrayMethodSort.js
function applyCustomSort() {
[].__proto__.sort2 = function(compareFunction) {
// write code here
[].__proto__.sort2 = function (compareFunction) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding an explicit type check for compareFunction so you throw a clear TypeError if a non-function is passed (the native Array.prototype.sort throws when compareFunction is provided but not a function). This check could go at the start of the function body before using compareFunction.

Comment thread src/arrayMethodSort.js
? (a, b) => compareFunction(a, b) > 0
: (a, b) => String(a) > String(b);

for (let i = 0; i < this.length; i++) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bubble sort is OK for this task, but it's O(n^2). For better performance on large arrays consider implementing a more efficient algorithm (quicksort/merge sort) or referencing the native sort for production code.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because your custom implementation is not yet wired so that arr.sort(...) actually uses it, which is a core requirement. In src/arrayMethodSort.js, you correctly define a bubble sort on [].__proto__.sort2, but Array.prototype.sort still points to the native implementation, so any call like someArray.sort() will not run your code [CHECKLIST ITEM #1, CHECKLIST ITEM #8]. To fix this, right after the line where you assign [].__proto__.sort2 = function(...) { ... }, add a delegation such as Array.prototype.sort = Array.prototype.sort2; (or a small wrapper like Array.prototype.sort = function(...args) { return this.sort2(...args); };). This single change will make all standard sort calls use your custom logic while preserving your existing compareFunction checks and bubble-sort behavior. As a minor improvement (not blocking), you could consider making the default comparator return negative/zero/positive numbers instead of a boolean to better match native sort, but this is optional for this task. Overall, the core sorting logic looks good; once you wire Array.prototype.sort to sort2, your solution will meet the assignment’s functional requirements.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/arrayMethodSort.js
}
}

return this;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You assign the implementation to [].__proto__.sort2 but do not delegate Array.prototype.sort to it. The task requires that calls to arr.sort(...) run your custom logic (checklist items #1 and #8). Add a delegation after defining sort2, for example:

Array.prototype.sort = Array.prototype.sort2;

or

Array.prototype.sort = function(...args) { return this.sort2(...args); };

Place this after the sort2 assignment so arr.sort(...) uses your implementation.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override so you can continue progressing, even though there are still a couple of issues to address. You did a solid job implementing a custom sort2 method with a bubble sort, handling comparator arguments, and wiring it so that arr.sort() uses your implementation, which shows good understanding of both prototypes and sorting logic. The main blocking issue is that ESLint reports no-extend-native in src/arrayMethodSort.js at line 30 (column 3), where you modify Array.prototype; the project’s rules forbid extending built-in prototypes, so you should instead expose your sort logic as a regular exported function that takes an array and returns a sorted copy (e.g., export const sortArray = (items, compareFn) => { ... }) and remove any Array.prototype.sort = ... and [].__proto__.sort2 = ... assignments. There is also a behavioral improvement opportunity around your comparator: currently, the result of compareFunction(a, b) is turned into a boolean using > 0 (around the comparison in your inner loop), which loses the full negative/zero/positive semantics of a native comparator; a better pattern is something like:

const cmp = compare(a, b);
if (cmp > 0) {
  // swap
}

and for the default comparator use something numeric, such as String(a).localeCompare(String(b)). Overall, your approach demonstrates clear understanding of sorting and prototypes; with the lint-compliant API change and the comparator refinement, your solution would closely match both the project rules and native Array.prototype.sort behavior—nice work so far, and keep iterating on these details.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/arrayMethodSort.js Outdated
}

const compare = compareFunction
? (a, b) => compareFunction(a, b) > 0
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation converts the compareFunction result to a boolean via > 0, losing the numeric semantics of a native comparator. Consider making compare return the comparator's numeric result (or use String.prototype.localeCompare for the default) and then use if (compare(this[j], this[j + 1]) > 0) { ... } to decide swaps. This is an optional improvement — not required for the task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants