Skip to content

feat: update merge_sort.ts #126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 1, 2023
Merged

Conversation

benben6515
Copy link
Contributor

  • use JavaScript ES6 syntax
  • use let, const instead of var
  • make it more "clean", I supposed

- use JavaScript ES6 syntax
- use `let`, `const` instead of `var`
- make it more "clean", I supposed
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

I support replacing var with let/const. However your "shortening" comes at the cost of efficiency:

  • You are copying both the result array and the remaining slices to a new final array.
  • You are not allocating the proper size for result anymore.

You also introduced a minor behavioral change: Before, mergeSort would always return a copy. Now it will return the same array if the length is <= 1. That is, it is "in-place" when the problem is trivial, and copies otherwise. E.g. the following would work:

const a = [42];
const b = mergeSort(a);
b[0] = 101;
console.log(a[0]); // 101

but this wouldn't:

const a = [33, 42];
const b = mergeSort(a);
b[0] = 101;
console.log(a[0]); // 33

Also please fix the test by applying the rename there too.

- fix merge_sort with return copy of array
- rename merge_sort.ts `MergeSrot` => `mergeSort`
- rename quick_sort.ts `QuickSrot` => `quickSort`
- rename merge_sort.test.ts `MergeSrot` => `mergeSort`
- rename quick_sort.test.ts `QuickSrot` => `quickSort`
@benben6515
Copy link
Contributor Author

Sorry about the unconsidered 😅

I just saw the original JavaScript's implement and converted it to TypeScript version.

https://the-algorithms.com/algorithm/merge-sort?lang=javascript

It truly comes at the cost of efficiency as you mentioned.

@benben6515
Copy link
Contributor Author

BTW, I also have another version of quickSort.

Does it make sense?

function quickSort(array: number[]): number[] {
  if (array.length <= 1) return array.slice()

  const pivot = array[0]
  const lesser: number[] = []
  const greater: number[] = []

  for (let i = 1; i < array.length; i++) {
    if (array[i] < pivot) lesser.push(array[i])
    else greater.push(array[i])
  }

  return quickSort(lesser).concat(pivot).concat(quickSort(greater))
}

const arr = [3.14, 5, -2, 1, 10]
console.log(quickSort(arr))
// [ -2, 1, 3.14, 5, 10 ]

@appgurueu
Copy link
Contributor

BTW, I also have another version of quickSort.

Does it make sense?

Yes, this is the "out of place" version which is easier to write but wastes memory. I don't think it should replace the in-place version. It might exist alongside it since it is more newbie-friendly.

@benben6515
Copy link
Contributor Author

After rewriting, I finally got your point! I updated merge_sort.ts again ~

And here is not "out of place" version of quick_sort, but it is very similar to the in-place version already. 😁

function quickSort(array: number[], left: number = 0, right: number = array.length - 1) {
  if (left >= right) return
  const index = partition(array, left, right)
  quickSort(array, left, index - 1)
  quickSort(array, index + 1, right)
}

function partition(array: number[], left: number, right: number) {
  const pValue = array[right]
  let pIndex = left
  for (let i = left; i < right; i++) {
    if (array[i] < pValue) {
      const temp = array[i]
      array[i] = array[pIndex]
      array[pIndex] = temp
      pIndex++
    }
  }
  const temp = array[right]
  array[right] = array[pIndex]
  array[pIndex] = temp
  return pIndex
}

const arr = [3.14, 5, -2, 1, 10]
quickSort(arr)
console.log(arr)
// [ -2, 1, 3.14, 5, 10 ]

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Can you get rid of the (unrelated?) changes to Quick Sort (or repurpose your PR accordingly)?

@benben6515
Copy link
Contributor Author

Sure!, I converted the naming of quick sort 😁

@raklaptudirm raklaptudirm merged commit 2603b22 into TheAlgorithms:master May 1, 2023
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