-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
Add in-place sort to QuickSort.js #16
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 77 78 +1
Lines 1602 1625 +23
Branches 283 289 +6
=====================================
+ Hits 1602 1625 +23
Continue to review full report at Codecov.
|
I was wrestling around with the lint requirements, and trying to work around the no-param-reassign rule when doing the algorithm in-place. I think the result is a bit messier than I would have liked it, but I also don't imagine you'd want to change the linting rules for this. |
Your contribution looks good. I have a question about the swap function.
to
? |
@chrisVillanueva I've actually never seen that implementation before; that's very cool. I don't think there is a huge performance implication either way (tests below), but I do think using a destructured approach is clearer, since it doesn't require a tempVariable.
|
@robtaussig thank you for in-place implementation! And also thank you for tests and clean code! |
I've always been a big fan of the in-place variation of the Quicksort algorithm. Not sure if you were looking for variations on existing implementations, but I thought it was different enough to warrant a pull request if you are interested.
It was also more performant when testing on arrays of 10,000 elements for integers between 0-10,000: