views:

377

answers:

1

I'm trying to implement quicksort in a functional style using C# using linq, and this code randomly works/doesn't work, and I can't figure out why.
Important to mention: When I call this on an array or list, it works fine. But on an unknown-what-it-really-is IEnumerable, it goes insane (loses values or crashes, usually. sometimes works.)
The code:

   public static IEnumerable<T> Quicksort<T>(this IEnumerable<T> source)
        where T : IComparable<T>
    {
        if (!source.Any())
            yield break;
        var pivot = source.First();
        var sortedQuery = source.Skip(1).Where(a => a.CompareTo(source.First()) <= 0).Quicksort()
                          .Concat(new[] { pivot })
                          .Concat(source.Skip(1).Where(a => a.CompareTo(source.First()) > 0).Quicksort());
        foreach (T key in sortedQuery)
            yield return key;
    }

Can you find any faults here that would cause this to fail?

Edit: Some better test code:

        var rand = new Random();
        var ienum = Enumerable.Range(1, 100).Select(a => rand.Next());
        var array = ienum.ToArray();
        try
        {
            array.Quicksort().Count();
            Console.WriteLine("Array went fine.");
        }
        catch (Exception ex)
        {
            Console.WriteLine("Array did not go fine ({0}).", ex.Message);
        }
        try
        {
            ienum.Quicksort().Count();
            Console.WriteLine("IEnumerable went fine.");
        }
        catch (Exception ex)
        {
            Console.WriteLine("IEnumerable did not go fine ({0}).", ex.Message);
        }
+6  A: 

Some enumerable instances, such as those returned by Linq to SQL or Entity Framework queries, are only designed to be iterated once. Your code requires multiple iterations and will crash or behave strangely on these types of instances. You'd have to materialize those enumerables with ToArray() or a similar method first.

You should also be reusing that pivot so that you don't have to keep iterating for the first and remaining elements. This may not completely solve the problem, but it'll help in some cases:

public static IEnumerable<T> Quicksort<T>(this IEnumerable<T> source)
    where T : IComparable<T>
{
    if (!source.Any())
        return source;
    var pivot = source.First();
    var remaining = source.Skip(1);
    return remaining
        .Where(a => a.CompareTo(pivot) <= 0).Quicksort()
        .Concat(new[] { pivot })
        .Concat(remaining.Where(a => a.CompareTo(pivot) > 0).Quicksort());
}

(You also don't need to iterate through the sortedQuery - just return it, it's already an IEnumerable<T>.)

On a related note, why do you feel the need to re-implement this functionality? Enumerable.OrderBy already does it for you.


Response to update:

Your tests are failing because your test is wrong, not the algorithm.

Random is a non-deterministic input source and, as I have explained above, the sort method needs to perform multiple iterations over the same sequence. If the sequence is totally random, then it is going to get different values on subsequent iterations. Essentially, you are trying to quicksort a sequence whose elements keep changing!

If you want the test to succeed, you need to make the input consistent. Use a seed for the random number generator:

static IEnumerable<int> GetRandomInput(int seed, int length)
{
    Random rand = new Random(seed);
    for (int i = 0; i < length; i++)
    {
        yield return rand.Next();
    }
}

Then:

static void Main(string[] args)
{
    var sequence = GetRandomInput(248917, 100);
    int lastNum = 0;
    bool isSorted = true;
    foreach (int num in sequence.Quicksort())
    {
        if (num < lastNum)
        {
            isSorted = false;
            break;
        }
        lastNum = num;
    }
    Console.WriteLine(isSorted ? "Sorted" : "Not sorted");
    Console.ReadLine();
}

It will come back sorted.

Aaronaught
My enumerable was actually just Enumerable.Range, and it still failed.Also, I tried just returning the sortedQuery, but I get an error - "Cannot return a value from an iterator. Use the yield return statement to return a value, or yield break to end the iteration."And - And - I don't need to implement this, I just want to, trying to learn functional programming.
Rubys
@Rubys: You're right about the "Cannot return a value" error - I just fixed that, the problem with that was the `yield break` at the start that was being mixed with a direct return at the end. I'll try this with `Enumerable.Range` and see what happens.
Aaronaught
@Rubys: Works absolutely fine on `Enumerable.Range` over here. Post your test code that's failing.
Aaronaught
Updated main post to include test code. Enuerable.Range seems to work, it did fail once, might have been my mistake, but the test I wrote now definitely fails more than wins.
Rubys
Well, that makes sense. Ironic, because it failed in a very failed-to-understand-functional way, while I was trying to accomplish the opposite. Thank you :)
Rubys