Recursive QuickSort suffering a StackOverflowException -- Need fresh eyes

Question

I am working on a Recursive QuickSort method implementation in a GenericList Class. I will have a second method that accepts a compareDelegate to compare different types, but for development purposes I'm sorting a GenericList<int>

I am recieving stackoverflow areas in different places depending on the list size.

I've been staring at and tracing through this code for hours and probably just need a fresh pair of (more experienced)eyes. Definitely wanting to learn why it is broken, not just how to fix it.

    public void QuickSort()
{
    int i, j, lowPos, highPos, pivot;
    GenericList<T> leftList = new GenericList<T>();
    GenericList<T> rightList = new GenericList<T>();
    GenericList<T> tempList = new GenericList<T>();

    lowPos = 1; highPos = this.Count;
    if (lowPos < highPos)
    {
        pivot = lowPos;
        for (i = 2; i <= highPos; i++)
        {
            if (this[i].CompareTo(this[pivot]) <= 0)
                leftList.Add(this[i]);
            else
                rightList.Add(this[i]);
        }
        leftList.Add(this[pivot]);
        leftList.QuickSort();
        rightList.QuickSort();

        for(i=1;i<=leftList.Count;i++)
            tempList.Add(leftList[i]);
        for(i=1;i<=rightList.Count;i++)
            tempList.Add(rightList[i]);

        this.items = tempList.items;
        this.count = tempList.count;
    }

}

//Finished Product

    public void QuickSort()
{
    Random random = new Random();
    int i, j, lowPos, highPos, pivot;
    GenericList<T> leftList = new GenericList<T>();
    GenericList<T> rightList = new GenericList<T>();
    GenericList<T> tempList = new GenericList<T>();

    if (this.Count > 1)
    {
        pivot = random.Next(1,this.Count);
        for (i = 1; i <= this.Count; i++)
        {
            if (i == pivot) continue;
            if (this[i].CompareTo(this[pivot]) < 0)
                leftList.Add(this[i]);
            else
                rightList.Add(this[i]);
        }
        leftList.QuickSort();
        rightList.QuickSort();

        for(i=1;i<=leftList.Count;i++)
            tempList.Add(leftList[i]);
        tempList.Add(this[pivot]);
        for(i=1;i<=rightList.Count;i++)
            tempList.Add(rightList[i]);

        this.items = tempList.items;
        this.count = tempList.count;
    }

}

Answer 1

You might get some insight from this series of blogs:

http://reprog.wordpress.com/2010/04/25/writing-correct-code-part-1-invariants-binary-search-part-4a/

http://reprog.wordpress.com/2010/04/27/writing-correct-code-part-2-bound-functions-binary-search-part-4b/

http://reprog.wordpress.com/2010/04/30/writing-correct-code-part-3-preconditions-and-postconditions-binary-search-part-4c/

When you're done with that, trace through your code in your mind with the input data set {1, 2} and tell me what the result will be.

Answer 2

Look what happens when you have a List containing [5,2]. Your pivot will equal 1, so the value used to compare will be 5. The line this[i].CompareTo(this[pivot]) <= 0 will be True, and the number 5 will be placed in leftList. Your next comparison with 2 will also be True, placing the 2 in leftList. Now your leftList will be exactly what it started with: [5,2], which you will call QuickSort with all over again...and it will come out exactly the same ad nausem: StackOverflow.

Answer 3

I wouldn't recommend putting the pivot into one of the two groups. If you have just two elements that are equal, you can get an infinite loop. For instance, if you try to sort the array {1,1}, you should get an infinite loop which could be the cause of your stack overflow. Most quicksorts avoid this by swapping the pivot with an element on the edge and then sorting the rest of the array. Another way to handle this would be to put in a line to make sure that you don't add the pivot to the leftList like

if(i != pivot)

Then you'd add the pivot to the tempList in between adding the leftList and adding the rightList.

Edit:

Nick is correct in that you will get the problem for other cases like {5,2}. However, even if you fix the current problem by not putting the pivot into a list that will be sorted again, you might want to make sure that your code can handle repeated elements. An large array of all the same number will give you O(N^2) time complexity, and if N is big enough, then you will get a stack overflow.

Answer 4

Your implementation is including the pivot in your sublists. By including the pivot in your sublists (in this case your left list because your condition is <=), you are setting yourself up for a possible infinite recursion if that pivot ends up in the middle of the sublist.

Example:

1. List = [3, 6, 12, 4, 8] Pivot = 3 (12) Left = [3, 6, 12, 4, 8] Right = [ Empty ]
2. List = [3, 6, 12, 4, 8] Pivot = 3 (12) Left = [3, 6, 12, 4, 8] Right = [ Empty ]
3. List = [3, 6, 12, 4, 8] Pivot = 3 (12) Left = [3, 6, 12, 4, 8] Right = [ Empty ]
4. ... Infinite Loop

Because the pivot is not excluded (although its final position is known), it can result in you sorting the same list over and over forever, rather than decreasing the size of the lists to sort with each recursive call.

If you exclude your pivot (by index, not by value) from the sublists and add it back it into the final tempList between leftList and rightList, it will work properly.

        ...
        for (i = 1; i <= highPos; i++)
        {
            if (i == pivot) continue; // Add this
            if (this[i].CompareTo(this[pivot]) <= 0)
        ...
        for (i = 1; i <= leftList.Count; i++)
            tempList.Add(leftList[i]);
        tempList.Add(this[pivot]); // Add this
        for (i = 1; i <= rightList.Count; i++)
            tempList.Add(rightList[i]);
        ...

See Also: Wikipedia article on Quicksort (with pseudocode)