Skip to content

Develop#2448

Open
TarasPidkuimukha wants to merge 2 commits into
mate-academy:masterfrom
TarasPidkuimukha:develop
Open

Develop#2448
TarasPidkuimukha wants to merge 2 commits into
mate-academy:masterfrom
TarasPidkuimukha:develop

Conversation

@TarasPidkuimukha
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

Excellent work on implementing your custom sort2 method! I am approving your solution as it correctly meets all the requirements. The bubble sort algorithm is well-implemented and handles both default sorting and a custom compareFunction. For future improvements, consider two points. First, the standard compareFunction for sort should return a number, not a boolean; using String(a).localeCompare(String(b)) is a good alternative. Second, you can optimize the bubble sort by changing the inner loop's condition to j < this.length - 1 - i to avoid re-checking sorted elements. These are just suggestions, not required changes. Keep up the great work!


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

Footnotes

  1. Rate AI review example

Comment thread src/arrayMethodSort.js
[].__proto__.sort2 = function(compareFunction) {
// write code here
[].__proto__.sort2 = function (
compareFunction = (a, b) => String(a) > String(b),
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 standard compareFunction for Array.prototype.sort is expected to return a number (negative, zero, or positive). Your function currently returns a boolean. While this works in your current implementation due to JavaScript's type coercion (true becomes 1, false becomes 0), it's better practice to adhere to the standard. This makes your code more predictable and reusable. Consider returning a number, for example by using String(a).localeCompare(String(b)).

Comment thread src/arrayMethodSort.js
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++) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your bubble sort implementation is correct. For a performance improvement, you can optimize the inner loop. After each pass of the outer loop, the largest unsorted element is moved to its correct position. Therefore, the inner loop doesn't need to iterate over the elements that are already sorted at the end of the array. You can adjust the inner loop's condition to j < this.length - 1 - i.

Copy link
Copy Markdown

@Denys-Kravchuk9988 Denys-Kravchuk9988 left a comment

Choose a reason for hiding this comment

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

Good job!
But I would recommend to apply all comments from AI.

  1. Sort function should return numbers (1, 0, -1)
  2. Performance fix

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