Quickselect implementation not working

Question

I am trying to write code to determine the n smallest item in an array. It's sad that I am struggling with this. Based on the algorithm from my college textbook from back in the day, this looks to be correct. However, obviously I am doing something wrong as it gives me a stack overflow exception.

My approach is:

Set the pivot to be at start + (end-start) / 2 (rather than start+end/2 to prevent overflow)
Use the integer at this location to be the pivot that I compare everything to
Iterate and swap everything around this pivot so things are sorted (sorted relative to the pivot)
If n == pivot, then I think I am done
Otherwise, if I want the 4 smallest element and pivot is 3, for example, then I need to look on the right side (or left side if I wanted the 2nd smallest element).

-

public static void main(String[] args) {
    int[] elements = {30, 50, 20, 10};
    quickSelect(elements, 3);
}

private static int quickSelect(int[] elements2, int k) {
    return quickSelect(elements2, k, 0, elements2.length - 1);
}

private static int quickSelect(int[] elements, int k, int start, int end) {
    int pivot = start + (end - start) / 2;
    int midpoint = elements[pivot];
    int i = start, j = end;

    while (i < j) {
        while (elements[i] < midpoint) {
            i++;
        }

        while (elements[j] > midpoint) {
            j--;
        }

        if (i <= j) {
            int temp = elements[i];
            elements[i] = elements[j];
            elements[j] = temp;
            i++;
            j--;
        }
    }
    // Guessing something's wrong here
    if (k == pivot) {
        System.out.println(elements[pivot]);
        return pivot;
    } else if (k < pivot) {
        return quickSelect(elements, k, start, pivot - 1);
    } else {
        return quickSelect(elements, k, pivot + 1, end);
    }
}

Edit: Please at least bother commenting why if you're going to downvote a valid question.

Please at least bother commenting why if you're going to downvote a valid question. — Beebunny, May 15 '16 at 08:29

score 1 · Answer 1 · answered May 15 '16 at 08:00

1

This won't fix the issue, but there are several problems with your code :

If you do not check for i < end and j > start in your whiles, you may run into out of bounds in some cases
You choose your pivot to be in the middle of the subarray, but nothing proves that it won't change position during partitioning. Then, you check for k == pivot with the old pivot position, which obviously won't work

Hope this helps a bit.

answered May 15 '16 at 08:00

T. Claverie

11,380
1
17
28

Your first bullet point is definitely good practice, but I can't see any case where this failure would occur. – Beebunny May 15 '16 at 08:15
I don't recall either, but I implemented a quicksort using a similar approach a few months ago and I remember I had trouble when those statements were missing. Though it may be specific to my implementation. – T. Claverie May 15 '16 at 08:17
See my updated solution below. Your response was very helpful. There is one additional issue with how I partition the array above. For values {30, 50, 20,10}, pivot starts at 1... 30 stays where it is, the left counter is incremented to 50, which gets swapped with 10 - just as you said, the supposed pivot moves... however, the routine ends with 30, 10, 20, 50.... even if 10 was the pivot or 50 was pivot, left side does not have all values less than pivot. FAIL. – Beebunny May 15 '16 at 08:28

score 0 · Accepted Answer · answered May 15 '16 at 08:22

Alright so the first thing I did was rework how I get my pivot/partition point. The shortcoming, as T. Claverie pointed out, is that the pivot I am using isn't technically the pivot since the element's position changes during the partitioning phase.

I actually rewrote the partitioning code into its own method as below. This is slightly different.

I choose the first element (at start) as the pivot, and I create a "section" in front of this with items less than this pivot. Then, I swap the pivot's value with the last item in the section of values < the pivot. I return that final index as the point of the pivot.

This can be cleaned up more (create separate swap method).

private static int getPivot(int[] elements, int start, int end) {
    int pivot = start;
    int lessThan = start;

    for (int i = start; i <= end; i++) {
        int currentElement = elements[i];
        if (currentElement < elements[pivot]) {
            lessThan++;
            int tmp = elements[lessThan];
            elements[lessThan] = elements[i];
            elements[i] = tmp;
        }
    }
    int tmp = elements[lessThan];
    elements[lessThan] = elements[pivot];
    elements[pivot] = tmp;

    return lessThan;
}

Here's the routine that's calls this:

private static int quickSelect(int[] elements, int k, int start, int end) {

    int pivot = getPivot(elements, start, end);

    if (k == (pivot - start + 1)) {
        System.out.println(elements[pivot]);
        return pivot;
    } else if (k < (pivot - start + 1)) {
        return quickSelect(elements, k, start, pivot - 1);
    } else {
        return quickSelect(elements, k - (pivot - start + 1), pivot + 1, end);
    }
}

Quickselect implementation not working

2 Answers2