1
\$\begingroup\$

I have been searching for ways to make my algorithm more efficient but most answers told me to divide the array into two differente arrays one of less elements and one of greater elements and then merge it, but that would make the QuickSort a non in-place sort anymore.

So, if someone could tell me a way of improving the efficiency of my code and if it is well designed or not, without affecting the in-place characteristic of the algorithm.

def QuickSort(array, right, left):    left_original = left    right_original = right    pivot_pos = right    while True:        for right in reversed(range(pivot_pos, right+1)):            if (array[right] < array[pivot_pos]):                break        for left in range(left, pivot_pos+1):            if (array[left] > array[pivot_pos]):                break        if right == left:            break        if array[pivot_pos] == array[right]:            pivot_pos = left        elif array[pivot_pos] == array[left]:            pivot_pos = right        array[right], array[left] = array[left], array[right]        right = right_original        left = left_original    right1 = pivot_pos - 1    left1 = left_original    right2 = right_original    left2 = pivot_pos + 1    if (right1 > left1):        QuickSort(array, right1, left1)    if (right2 > left2):        QuickSort(array, right2, left2)
Reinderien's user avatar
Reinderien
71.2k5 gold badges76 silver badges257 bronze badges
askedJan 4, 2022 at 20:15
Luigi_S_R's user avatar
\$\endgroup\$
1
  • 2
    \$\begingroup\$Hopefully this is just for learning, because the built-in sort should nearly always be preferred to hand-rolled code.\$\endgroup\$CommentedJan 4, 2022 at 22:12

1 Answer1

2
\$\begingroup\$

Fix your function signature. Most Python programmers use lowercase for functionnames. Even more important, don't swap the order the left and right:

# No: why is "left" to the right of "right"?!?def QuickSort(array, right, left):    ...# Yes: less chance for confusion.def quicksort(array, left, right):    ...

Don't make the caller know your implementation details. Forcing the callerto pass values for left and right is not convenient for your users; it issomewhat error prone (for example, in some cases you can pass the wrongindexes, the function will complete without error, but the list won't besorted); and it's not needed because Python makes it easy to define a functionwith optional arguments:

def quicksort(xs, left = 0, right = None):    if right is None:        right = len(xs) - 1    ...

Ranges have a step value. If you are usingrange(), you don't needreversed(). Use a negative step:

for right in range(right, pivot_pos - 1, -1):    ...

What about empty sequences?. Currently your code raises an error. Thisproblem is related to the next point.

Enforce recursive termination in one spot, almost always at the beginning.As a general rule for recursive functions, enforce the termination condition atthe outset -- and only there. Your code makes a very common mistake of trying tolook into the future: before making the recursive call, you check forpreconditions. I observed this mistake in many software engineeringinterviews, and it almost always caused the applicants to become moreconfused. The better approach is to be confident: just make the recursive calland let the function handle its own preconditions. Here's that pointillustrated with a code comparison:

# If we try to look into the future, additional variables# are spawned, increasing complexity.def quicksort(array, left, right):    ...    right1 = pivot_pos - 1    left1 = left_original    right2 = right_original    left2 = pivot_pos + 1    if (right1 > left1):        quicksort(array, left1, right1)    if (right2 > left2):        quicksort(array, left2, right2)# Things are simpler if we blindly recurse, letting the function# enforce preconditions in a natural manner.def quicksort(array, left, right):    if right <= left:        return    ...    quicksort(array, left_original, pivot_pos - 1)    quicksort(array, pivot_pos + 1, right_original)

Using top-down implementation: an alternative to consider. Although yourcurrent implementation appears to work correctly, I found it difficult toreason about. For example, it seems like the main while-true loop might bedoing repetitive work, because on every iterationright andleft are resetto the original boundaries -- but perhaps I overlooked a detail. Either way,the core idea of quicksort is very intuitive: we select a pivot value and makethe necessary swaps to enforce the quicksort invariant, namelysmall values < pivot value <= large values. But the implementation does not build on thatintuition or make it evident, either in code or in comments to guide thereader. The implementation is additionally complex because several variablesare changing at once:right is being pushed downward,left pushed upward,pivot_pos is sometimes altered based on those changes, then we swap a largevalue and a small value, and thenleft andright are reset. I guess thathas a resemblance to quicksort (and is probably correct) but the alignmentbetween the code and the intuition not super obvious. Sometimes when strugglingto write an intuitive implementation (as distinct from a merely working one), Iwill use a top-down approach. First we start with something clear and basic:

def quicksort(xs, i = 0, j = None):    # Set optional arguments.    if j is None:        j = len(xs) - 1    # Base case: do nothing if indexes have met or crossed.    if not i < j:        return    # Partition the sequence to enforce the quicksort invariant:    # "small values" < pivot value <= "large values". The function    # returns the index of the pivot value.    pi = partition(xs, i, j)    # Sort left side and right side.    quicksort(xs, i, pi - 1)    quicksort(xs, pi + 1, j)

Implement the functions that your top-level design omitted. So far, wehave simplicity and clarity, but only because we've deferred the trickiestpart: how to do the partitioning. But the top-down approach has a big benefitin that we've reduced the size the problem considerably: instead of doingsorting, we are merely partitioning; instead of build a user-facing function, weare writing a utility function intended for use only by our quicksorter; andinstead of building a recursive function, we are building a regular function.In my notes from various algorithm classes, I have a quicksort partitioningfunction along the following lines. Note how some of its simplicity comes fromthe fact that we are not modifying too many values:k which is just theprimary iteration variable, andpb which represents the currentpartitioning boundary. Also note how additional simplicity comes from writinganother utility: theswap() function. If we were writing a mission-criticalsort function and cared about raw speed, we would not use that approach;instead, we would do the swapping in line. However, in this case we arefocusing on algorithmic clarity. In that context, moving the grubby swappingdetails elsewhere is a benefit.

def partition(xs, i, j):    # Get the pivot value and initialize the partition boundary.    pval = xs[i]    pb = i + 1    # Examine all values other than the pivot, swapping to enforce the    # invariant. Every swap moves an observed "small" value to the left of the    # boundary. "Large" values are left alone since they are already to the    # right of the boundary.    for k in range(pb, j + 1):        if xs[k] < pval:            swap(xs, k, pb)            pb += 1    # Put pivot value between the two sides of the partition,    # and return that location.    swap(xs, i, pb - 1)    return pb - 1def swap(xs, i, j):    xs[i], xs[j] = xs[j], xs[i]
answeredJan 6, 2022 at 1:34
FMc's user avatar
\$\endgroup\$

You mustlog in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.