fix: adding median of three technique and module interface to QuickSortRecursive.js and its test file #1686

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

Jump to bottom

Open

stormkid2009 wants to merge 3 commits into TheAlgorithms:master

from stormkid2009:master

Open

fix: adding median of three technique and module interface to QuickSortRecursive.js and its test file #1686

stormkid2009 wants to merge 3 commits into TheAlgorithms:master from stormkid2009:master

Conversation

stormkid2009

stormkid2009 added 3 commits

September 15, 2024 21:37

@stormkid2009


 feat: add function to choose better pivot and interface for the module

f53cf76

@stormkid2009


 fix: refactor the test cases with adding interface for the module

cc07003

@stormkid2009


 fix: repair style with prettier

e573010

@stormkid2009 stormkid2009 requested review from raklaptudirm and appgurueu as code owners

September 17, 2024 15:09

@codecov-commenter

Copy link

codecov-commenter commented Sep 17, 2024

Codecov Report

Attention: Patch coverage is 96.92308% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.68%. Comparing base (9010481) to head (e573010).

Files with missing lines	Patch %	Lines
Sorts/QuickSortRecursive.js	96.92%	2 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@
## master #1686 +/- ##
==========================================
+ Coverage 84.65% 84.68% +0.02% 
==========================================
 Files 378 378 
 Lines 19744 19791 +47 
 Branches 2951 2957 +6 
==========================================
+ Hits 16715 16760 +45 
- Misses 3029 3031 +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

appgurueu

appgurueu reviewed

Oct 2, 2024

View reviewed changes

Sorts/QuickSortRecursive.js

* Sorts the input list using the quicksort algorithm.

* @param {Array} inputList - the list to be sorted

* @return {Array} the sorted list

Copy link

Collaborator

@appgurueu appgurueu Oct 2, 2024

Choose a reason for hiding this comment

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

Since we mutate the array anyways, it's unnecessary to return it, and IMO cleaner not to.

Copy link

Author

@stormkid2009 stormkid2009 Oct 4, 2024

Choose a reason for hiding this comment

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

You are absolutely right ,thanks for reminding me .

appgurueu

appgurueu reviewed

Oct 2, 2024

View reviewed changes

Sorts/QuickSortRecursive.js

@@ -43,23 +58,55 @@ const quickSort = (inputList, low, high) => {

* @returns {number} `pIndex` pivot index value.

const partition = (partitionList, low, high) => {

const pivot = partitionList[high]

const mid = Math.floor((low + high) / 2)

// get the median of three is good technique for partitioning to be sure that the 2 sub-arrays will be almost equal or nearly equal in size

Copy link

Collaborator

@appgurueu appgurueu Oct 2, 2024

Choose a reason for hiding this comment

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

This comment is phrased a bit poorly and also misleading: Nothing is guaranteed here. It's just a heuristic. It can still be horribly wrong if the arrays are crafted to be worst case.

Copy link

Author

@stormkid2009 stormkid2009 Oct 4, 2024

Choose a reason for hiding this comment

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

I am with you that nothing guaranteed but trying to improve something worth it.

appgurueu

appgurueu reviewed

Oct 2, 2024

View reviewed changes

Sorts/QuickSortRecursive.js

// Move pivot to the end

let pivotIndex = partitionList.indexOf(pivot)

;[partitionList[pivotIndex], partitionList[high]] = [

Copy link

Collaborator

@appgurueu appgurueu Oct 2, 2024

Choose a reason for hiding this comment

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

I would rename some variables to make this less verbose.

Copy link

Author

@stormkid2009 stormkid2009 Oct 4, 2024

Choose a reason for hiding this comment

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

ok that will be better

Labels

None yet

3 participants

@stormkid2009 @codecov-commenter @appgurueu

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: adding median of three technique and module interface to QuickSortRecursive.js and its test file #1686

Are you sure you want to change the base?

fix: adding median of three technique and module interface to QuickSortRecursive.js and its test file #1686

Uh oh!

Conversation

@stormkid2009 stormkid2009 commented Sep 17, 2024

Describe your change:

Checklist:

Uh oh!

codecov-commenter commented Sep 17, 2024

Codecov Report

Uh oh!

@appgurueu appgurueu Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

@stormkid2009 stormkid2009 Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

@stormkid2009 stormkid2009 Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

@stormkid2009 stormkid2009 Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!