Skip to content

Develop#2467

Open
PAVLO111 wants to merge 7 commits intomate-academy:masterfrom
PAVLO111:develop
Open

Develop#2467
PAVLO111 wants to merge 7 commits intomate-academy:masterfrom
PAVLO111:develop

Conversation

@PAVLO111
Copy link

No description provided.

Copy link

@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 a core requirement of the task is not yet implemented, which will prevent the solution from passing the tests. In src/arrayMethodSort.js, you correctly define [].__proto__.sort2, but you never assign Array.prototype.sort to delegate to this new method; the task explicitly requires that Array.prototype.sort be implemented using sort2, so this is a functional gap. To fix this, after the sort2 definition, add an implementation like Array.prototype.sort = function(compareFn) { return [].__proto__.sort2.call(this, compareFn); }; so that all array instances use your custom sort.

A smaller, non-blocking improvement: your default comparator currently only returns 1 or -1, even when String(a) === String(b), which differs from the native behavior and can cause unnecessary swaps. You can improve this by checking for equality first and returning 0 in that case, e.g. by comparing const A = String(a); const B = String(b); and returning 0 when A === B. Overall, your bubble sort implementation and use of the prototype are on the right track—once you wire Array.prototype.sort to sort2 and refine the default comparator, the solution will align well with the 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

}
}

return this;

Choose a reason for hiding this comment

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

The task requires that Array.prototype.sort be implemented using the predefined [].__proto__.sort2. This file only defines sort2 and never assigns or implements Array.prototype.sort. After the sort2 definition you should add an assignment such as:

Array.prototype.sort = function(compareFn) {
  return [].__proto__.sort2.call(this, compareFn);
};

This fixes the HIGH-severity missing requirement.

// write code here
[].__proto__.sort2 = function (compareFunction) {
const input =
compareFunction || ((a, b) => (String(a) > String(b) ? 1 : -1));

Choose a reason for hiding this comment

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

The default comparator ((a, b) => (String(a) > String(b) ? 1 : -1)) does not return 0 when String(a) === String(b). That can cause unnecessary swaps and differs from standard sort behavior. Consider returning 0 for equality, e.g.:

(a, b) => {
  const A = String(a);
  const B = String(b);
  if (A === B) return 0;
  return A > B ? 1 : -1;
}

This makes behavior closer to the native comparator.

Copy link

@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 there is a critical initialization-order bug that can break Array.prototype.sort at runtime. Right now, sort2 is defined inside applyCustomSort(), but the module assigns Array.prototype.sort = function(...) { return [].__proto__.sort2.call(this, ...) } at the top level, before applyCustomSort() has run. This means that if any code calls [].sort() before applyCustomSort() has been executed, [].__proto__.sort2 will still be undefined and the call will throw, which violates the requirement that your replacement sort functions correctly. To fix this, you can either: (1) move the Array.prototype.sort = ... assignment inside applyCustomSort() immediately after defining sort2, or (2) ensure applyCustomSort() is invoked once at module load time before any code can use .sort(). Once this wiring issue is resolved, the rest of your solution (custom sort2 bubble sort, correct default comparator returning 0 for equal values, and delegating Array.prototype.sort to sort2) looks solid, so you’re very close—this is just about making the installation of the custom sort safe and reliable.


✨ 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 on lines 39 to 41
Array.prototype.sort = function (compareFunction) {
return [].__proto__.sort2.call(this, compareFunction);
};

Choose a reason for hiding this comment

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

You define sort2 inside applyCustomSort() (this installs the custom implementation) but the Array.prototype.sort assignment below calls [].__proto__.sort2 regardless of whether applyCustomSort() has been run. That means calling .sort() before applyCustomSort() will fail. Consider moving the Array.prototype.sort = ... assignment into applyCustomSort() after sort2 is defined, or ensure applyCustomSort() is invoked before any .sort() use.

function applyCustomSort() {
[].__proto__.sort2 = function(compareFunction) {
// write code here
[].__proto__.sort2 = function (compareFunction) {

Choose a reason for hiding this comment

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

This is the sort2 definition. Keeping sort2 inside applyCustomSort() is fine, but be aware of the initialization order described above — Array.prototype.sort must be wired to this implementation only once sort2 exists.

Copy link

@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 keep progressing while you refine your skills. You correctly define a custom sort2 on Array.prototype and then reassign Array.prototype.sort to delegate to sort2 using sort2.call(this, compareFunction), which matches the requirement to wire everything through the prototype method. Your implementation sorts in place and includes a default comparator that properly returns 0 when values are equal, which is exactly what the native behavior expects.

One improvement to consider is being more defensive about the compareFunction parameter: right now, if someone calls array.sort('not a function'), it will throw when you try to call it—adding a typeof compareFunction === 'function' check and falling back to your default comparator would make it more robust. You might also decide whether you want your applyCustomSort() to run automatically when the module loads or stay as an explicit function that callers must invoke, depending on how you expect it to be used. Overall, this is solid work that meets the main requirements and shows good understanding of how to extend Array.prototype; keep iterating on robustness and edge cases as your next learning step.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

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.

2 participants