What is wrong with this QuickSort algorithm in C#?

Question

Sadly, I've got problems with Quicksort as well. I've discussed it with other students and tried the standard troubleshooting methods. But I can't find what I'm doing wrong...

static int partition(int[] A)
    {
        int pivot, i, j;
        i = 0;
        j = A.Length;
        pivot = A[i];

        do
        {
            do
            {
                i++;
            }
            while (A[i] < pivot || i < j);
            do
            {
                j--;
            }
            while (A[j] > pivot);

            swap(ref A[i], ref A[j]);
        }
        while (i < j);

        if (i <= j)
        {
            swap(ref A[i], ref A[j]);
            swap(ref A[0], ref A[j]);
        }

        return j;
    }

    static void swap(ref int a, ref int b)
    {
        int aCopy = a;
        a = b;
        b = aCopy;
    }

    static int[] QuickSort(int[] A)
    {
        int s;
        int left = 0;
        int right = A.Length;
        int[] B, C;

        if (A.Length == 0 || A.Length == 1)
            return A;
        else
        {
            s = partition(A);
            B = new int[s];
            C = new int[right - s];

            for (int i = left; i < s; i++)
            {
                B[i - left] = A[i];
            }
            for (int i = s; i < right; i++)
            {
                C[i - s] = A[i];
            }

            QuickSort(B);
            QuickSort(C);
            return A;
        }
    }

The debugger constantly gives me an 'IndexOutOfRangeException' with the j variable. I've tried adjusting the

while (A[i] < pivot || i < j);

part. But that didn't do anything (well, it resulted in a StackOverflowException once).

The Homework tag was recently depreciated. I removed it for you. — Eric J.
– Eric J., Commented Oct 8, 2012 at 16:41
That means you are trying to access an index in your array which doesn't actually exist. For example, if your array has 3 elements, you are trying to access array[4]. — Tejas Sharma
– Tejas Sharma, Commented Oct 8, 2012 at 16:42
Along with the notes below notice that you are executing QuickSort passing in (B) and (C), but you are throwing away the results of those two QuickSorts. You will need to figure out a way to retain those results. — Gene S
– Gene S, Commented Oct 8, 2012 at 17:03

Justin Niessner · Accepted Answer · 2012-10-08 16:42:09Z

3

You have j = A.Length, but it should really be j = A.Length - 1 (based on the loops you are using).

answered Oct 8, 2012 at 16:42

Justin Niessner

246k40 gold badges416 silver badges546 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Nikola Davidovic · Accepted Answer · 2012-10-08 17:53:33Z

Well, there are too many mistakes, like you are swapping back the elements you have already swapped and so on. This is your solution with your way of doing it without mistakes that also includes what has been said in previous answers and comments. Nice coding!

    static int partition(int[] A)
    {
        int pivot, i, j;
        i = 0;
        j = A.Length-1;
        pivot = A[0];

        while (i < j)
        {
            while (A[i] < pivot && i < j)
            {
                i++;
            }

            while (A[j] > pivot && j > i)
            {
                j--;
            }
            swap(ref A[i], ref A[j]);
        }

        return j;
    }

    static void swap(ref int a, ref int b)
    {
        int aCopy = a;
        a = b;
        b = aCopy;
    }

    static int[] QuickSort(int[] A)
    {
        int s;
        int left = 0;
        int right = A.Length;
        int[] B, C;

        if (A.Length == 0 || A.Length == 1)
            return A;
        else
        {
            s = partition(A);
            B = new int[s];
            C = new int[right - s - 1];

            for (int i = left; i < s; i++)
            {
                B[i - left] = A[i];
            }
            for (int i = s + 1; i < right; i++)
            {
                C[i - s - 1] = A[i];
            }

            B = QuickSort(B);
            C = QuickSort(C);
            for(int i = 0; i < s;i++)
                A[i] = B[i - left];
            for (int i = s + 1; i < right; i++)
            {
                A[i] = C[i - s - 1];
            }
            return A;
        }
    }

Thanks! Looking at this, it becomes obvious why I got the errors. I'm a little bit amazed that I could allow myself to make these mistakes. Anyhoo, thanks, again.

Guffa · Accepted Answer · 2012-10-08 16:54:52Z

1

You are always including the first item as one under the pivot without checking the value, then you are including all the other values even if they are above the pivot, just because i < j, as you use the 'or' operator (||) between the conditions.

This:

do
{
  i++;
}
while (A[i] < pivot || i < j);

Should be:

while (i < j && A[i] < pivot);
{
  i++;
}

answered Oct 8, 2012 at 16:54

Guffa

703k111 gold badges760 silver badges1k bronze badges

Comments

Charles Bretana · Accepted Answer · 2012-10-08 22:05:53Z

I notice that you are still trying tio use Quicksort even when the segments are 2 or 3 elements long. I think you need to use an alternate sort technique when you have reduced the size of the input arrays to less than 4 elements. Remember, the quick sort requires that you break up the input array into a "pivot" element, and 2 separate arrays, one with all the elements less than the pivot, and the other with all the elements greater than the pivot. To do this the input array has to have at least 3 elements. This may be the source of your issue.

Here's a generic quicksort that uses this technique...

public delegate int CompareDlg<T>(T thisOne, T otherOne);

public class QuickSort<T> 
{
    #region private variable to sort inplace
    readonly private IList<T> itms;
    private CompareDlg<T> cmp;
    #endregion private variable to sort inplace

    #region properties
    public CompareDlg<T> Comparer
    {
        set { cmp = value; }
        get { return cmp; }
    }
    #endregion properties

    #region ctor
    private QuickSort() { } //     Hide parameterless constructor
    /// <summary>
    /// Constructor, requires generic List of Ts 
    /// where T must implement CompareTo() method...
    /// </summary>
    /// <param name="list">List of T elements to sort</param>
    public QuickSort(IList<T> list) : this(list, null) { }
    /// <summary>
    /// Constructor, requires generic List of Ts 
    /// where T must implement CompareTo() method,
    /// And Compare method to use when sorting,
    /// (Overrides default CompareTo() implemented by T) ...
    /// </summary>
    /// <param name="list">List of T elements to sort</param>
    /// <param name="compareDelegate">Method to use to compare elements</param>
    public QuickSort(IList<T> list, CompareDlg<T> compareDelegate)
        : this()
    {
        if (list.Count == 0) throw new InvalidOperationException(
            "Empty List passed to QuickSort.");
        var first = default(T);
        if (typeof(T).IsClass)
        {
            foreach (var t in list)
                if (!((first = t).Equals(default(T))))
                    break;

            if (first.Equals(default(T)))
                throw new InvalidOperationException(
                    "List passed to QuickSort contains all nulls.");
        }
        if (compareDelegate == null && !(first is IComparable<T>))
            throw new InvalidOperationException(string.Format(
                "Type {0} does not implement IComparable<{0}>. " +
                "Generic Type T must either implement IComparable " +
                "or a comparison delegate must be provided.", typeof(T)));
        itms = list;
        cmp += compareDelegate ?? CompareDefault;
    }
    #endregion ctor

    #region public sort method
    public static void Sort(IList<T> itms) { (new QuickSort<T>(itms)).Sort(); }
    public void Sort(bool asc) { Sort(0, itms.Count - 1, asc); }
    public static void Sort(IList<T> itms, CompareDlg<T> compareDelegate)
    { (new QuickSort<T>(itms, compareDelegate)).Sort(); }
    public void Sort() { Sort(0, itms.Count - 1, true); }
    /// <summary>
    /// Executes QuickSort algorithm
    /// </summary>
    /// <param name="L">Index of left-hand boundary of partition to sort</param>
    /// <param name="R">Index of right-hand boundary of partition to sort</param>
    /// <param name="asc"></param>
    private void Sort(int L, int R, bool asc)
    {
        // Call iSort (insertion-sort) 
        if (R - L < 4) iSort(L, R);
        //for partitions smaller than 4 elements
        else
        {
            int i = (L + R) / 2, j = R - 1;
            // Next three lines to set upper and lower bounds
            if (Comparer(itms[L], itms[i]) > 0 == asc) Swap(L, i);
            if (Comparer(itms[L], itms[R]) > 0 == asc) Swap(L, R);
            if (Comparer(itms[i], itms[R]) > 0 == asc) Swap(i, R);
            Swap(i, j);
            // --------------------------------------------
            var p = itms[j]; // p = itms[j] is pivot element
            i = L;
            while (true)
            {
                while (Comparer(itms[++i], p) < 0 == asc) { }
                while (Comparer(itms[--j], p) > 0 == asc) { }
                if (j < i) break;
                Swap(i, j);
            }
            Swap(i, R - 1);
            Sort(L, i, asc);     // Sort  Left partition
            Sort(i + 1, R, asc); // Sort Right partition
        }
    }
    private static int CompareDefault(T thisOne, T otherOne)
    {
        if(!(thisOne is IComparable<T>)) 
            throw new InvalidCastException(
                "Type must implement IComparable<T>");
        return (thisOne as IComparable<T>).CompareTo(otherOne);
    }
    #endregion public sort method

    #region private Helper methods
    private void Swap(int L, int R)
    {
        var t = itms[L];
        itms[L] = itms[R];
        itms[R] = t;
    }
    private void iSort(int L, int R)
    {
        for (var i = L; i <= R; i++)
        {
            var t = itms[i];
            var j = i;
            while ((j > L) && Comparer(itms[j - 1], t) > 0)
            {
                itms[j] = itms[j - 1];
                j--;
            }
            itms[j] = t;
        }
    }
    #endregion private Helper methods
}

Collectives™ on Stack Overflow

What is wrong with this QuickSort algorithm in C#?

4 Answers 4

Comments

1 Comment

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Comments

1 Comment

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related