Trying to Do Quicksort in Place

Question

Seems like it should be pretty straightforward, I've done quick sort before, but I've got some bug in my code, and I can't quite find it. Here's my code:

public static void quickSortInPlace(ArrayList<Integer> numbers, int left, int right) {
    if(left < right) {
        int index = generatePivot(numbers);
        index = partition(numbers, left, right, index);
        quickSortInPlace(numbers, left, index - 1);
        quickSortInPlace(numbers, index + 1, right);
    }
}

public static int partition(ArrayList<Integer> numbers, int left, int right, int index) {
    int pivot = numbers.get(index);
    int tmp = numbers.get(right);
    numbers.set(right, numbers.get(index));
    numbers.set(index, tmp);
    int newIndex = left;
    for(int i = left; i < right; i++) {
        if(numbers.get(i) <= pivot) {
            tmp = numbers.get(i);
            numbers.set(i, numbers.get(newIndex));
            numbers.set(newIndex, tmp);
            newIndex++;
        }
    }
    tmp = numbers.get(newIndex);
    numbers.set(newIndex, numbers.get(right));
    numbers.set(right, tmp);
    return newIndex;
}

//Chooses an index in the array for the pivot value. Randomly picks 3 values from the array, and chooses the intermediate value. This is simply an optimization.
public static int generatePivot(ArrayList<Integer> numbers) {
    Random rand = new Random();
    int rand1 = rand.nextInt(numbers.size()), rand2 = rand.nextInt(numbers.size()), rand3 = rand.nextInt(numbers.size());
    int pivot1 = numbers.get(rand1), pivot2 = numbers.get(rand2), pivot3 = numbers.get(rand3);
    int max = Math.max(Math.max(pivot1, pivot2), pivot3);
    int min = Math.min(Math.min(pivot1, pivot2), pivot3);
    int pivot = pivot1 ^ pivot2 ^ pivot3 ^ max ^ min;
    int index = 0;
    if(pivot == pivot1) {
        index = rand1;
    } else if(pivot == pivot2) {
        index = rand2;
    } else if(pivot == pivot3) {
        index = rand3;
    }
    return index;
}

I'm giving the method quickSortInPlace an ArrayList filled with 10,000 randomly generated integers simply for the purpose of checking the correctness of the code. I would expect it to spit out an ordered ArrayList of 10,000 integers.

I've been looking over it for like 45 minutes now, so I'm sure it's something stupid, and I just can't see it because I've been looking at it so long. It just returns utter garbage. Any help would be greatly appreciated. Thanks!

The body of `generatePivot(al)`, some example input, and expected output would be helpful. — aliteralmind, Feb 20 '14 at 02:55
What's the point of doing those XORs while choosing the pivot? The pivot should be close to median in order to give expected O(n*logn) complexity. You're doing nothing to find the pivot closest to median, you could just take the last element just like in legacy quicksort version — mangusta, Feb 20 '14 at 03:33
In my understanding of Quick Sort, it exhibits worst case behavior when the pivot value happens to be either the largest, or the smallest, value in the array. By picking 3 values, and choosing the intermediate value of the three, I guarantee that the chosen value cannot be the smallest nor the largest in the array, and thereby guarantee that, at the least, it will perform better than worst case time. I apologize for the XORs, I probably should have put a comment there explaining it, but it saves me the otherwise necessary large if-block to locate the intermediate value. — Chris Fretz, Feb 20 '14 at 04:09
But at any rate, a proper implementation of Quick Sort will succeed in sorting a given array regardless of what the chosen pivot is, so that can't be the reason that this code is returning garbage. — Chris Fretz, Feb 20 '14 at 04:11
I suppose I should state that, although the XOR's look strange, that line is actually guaranteed to return the median value. — Chris Fretz, Feb 20 '14 at 04:19
As for me, that pivot selection procedure is far more interesting than the `partition()` :) But I doubt that it guarantees median value — mangusta, Feb 20 '14 at 05:31
Lol, thanks. I guess saying "guarantee" might be a bit much, but in all of my testing with it so far it's always yielded the median value. — Chris Fretz, Feb 20 '14 at 07:16

score 0 · Answer 1 · answered Feb 20 '14 at 05:25

0

I haven't gotten through the whole thing, but it looks to me like generatePivot isn't guaranteed to return a number between left and right.

answered Feb 20 '14 at 05:25

Roy

974
6
11

Thanks for taking a look through. I know that I've used some unorthodox syntax in generatePivot, but, at least in all of my testing, it consistently works as described. Either way, I never get indexOutOfBounds exceptions, so even if it isn't picking the pivot as I believe it is, it's at least always picking a value contained within left and right. It's just that the end result is an array that isn't ordered properly. – Chris Fretz Feb 20 '14 at 07:21

Trying to Do Quicksort in Place

1 Answers1