Exception with LINQ OrderBy

Question

My linq query is as follows:

var agreementsMatching = _ctx.LeasingAgreements
    .Where(x => x.Dealer.Id == dealer.dealerId)
    .ToList();
var ag = agreementsMatching
    .OrderBy(o => o.Model.Specification)
    .OrderBy(o => o.Model.ModelName)
    .OrderBy(o => o.Model.ModelBrand)
    .OrderBy(c => c.LeasingAgreementClicks)
    .GroupBy(sg => sg.Model.Specification)
    .Select(sg => new { GroupId = sg.Key, Agreements = sg });

The reason I think it might not be the best query is also that it gives me an exception:

At least one object must implement IComparable.

I do know that it occurs because one or more objects are not implementing the IComparable interface. I'm just not sure on how to actually handle it.

---

Edit: It turned out that I didn't need all those OrderBy calls. I could simply do this:

var agreementsMatching = _ctx.LeasingAgreements
    .Where(x => x.Dealer.Id == dealer.dealerId)
    .ToList();
var ag = agreementsMatching
    .GroupBy(sg => sg.Model.Specification)
    .Select(sg => new { GroupId = sg.Key, Agreements = sg });

Though the problem is solved now, I still would like to learn how to avoid the aforementioned error :)

Answer 1

The first thing to note is that

someList.OrderBy(item => item.SomeProp).OrderBy(item => item.SomeOtherProp);

Is more or less equivalent to:

someList.OrderBy(item => item.SomeOtherProp);

Because the second OrderBy undoes the work of the first. Generally you want:

someList.OrderBy(item => item.SomeProp).ThenBy(item => item.SomeOtherProp);

Note that the equivalent:

from item in someList orderby item.SomeProp, item.SomeOtherProp select item

Makes use of ThenBy as above.

Now, with either syntax, with Linq-to-objects (but not database and other linq query providers) OrderBy works through a call to IComparable<T> if available, and IComparable otherwise (barring an exception we'll come to later). Since agreementsMatching is a list in memory, this is the form used. This is how OrderBy can know what "order by" means for a given type.

Strings, and all the built-in numeric types (int, double, etc), implement IComparable<T> so they can all be used fine without any action from you.

Presumably at least one of the properties you were ordering by above wasn't one of these types, but a type of your own. I can't tell which from your code, so I'm going to make up the following:

I'm going to assume that the Specification property returned a Spec object, and that Spec objects should be ordered by their Name property in a case-insensitive manner according to the invariant culture. So I'm starting with:

class Spec
{
  public property Name
  {
     get { /* code I don't care about here*/ }
     set { /* code I don't care about here*/ }
  }
  /* more code I don't care about here*/
}

I add an implementation of IComparable<Spec>. It's also a good idea to implement IComparable in such cases, for backwards compatibility, though it could be skipped. Both define a method that compares the instance with another object, returning a number < 0 if the instance is "lesser" (comes first in order), 0 if they are equivalent in ordering and >0 if the instance is "greater":

class Spec : IComparable<Spec>, IComparable
{
  public property Name
  {
     get { /* code I don't care about here*/ }
     set { /* code I don't care about here*/ }
  }
  /* more code I don't care about here*/
  public int CompareTo(Spec other)
  {
     if(other == null)
       return 1;
     //Often we make use of an already-existing comparison, though not always
     return string.Compare(Name, other.Name, StringComparison.InvariantCultureIgnoreCase)
  }
  //For backwards compatibility:
  public int CompareTo(object other)
  {
    if(other == null)
      return 1;
    Spec os = other as Spec;
    if(os == null)
      throw new ArgumentException("Comparison between Spec and " + other.GetType().FullName + " is not allowed");
    return CompareTo(os);
  }
}

Now, OrderBy can handle comparisons of Spec objects, which it will do by name (also, we can use List<Spec>.Sort() and a whole bunch of other things.

A final matter, is what happens if we need to sort by some other rules, or if we need to sort a type where we don't have the source code and it doesn't implement IComparable<T> or IComparable?

Here we can create a class that implements IComparer<T>, and this will bypass the use of IComparable<T>. Here's an example that demonstrates this:

public class OddBeforeEven : IComparer<int>
{
  public int Compare(int x, int y)
  {
    int compareOddEven = y % 2 - x % 2;
    if(compareOddEven != 0)
      return compareOddEven;
    //if both odd or both even, use default ordering:
    return x.CompareTo(y);
  }
}
/* ... */
var oddBeforeEven0To20 = Enumerable.Range(0, 21).OrderBy(x => x, new OddBeforeEven());
/*Enumerating oddBeforeEven0To20 will produce 1, 3, 5, 7, 9, 11, 13, 15, 17, 19, 0, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20*/

A final note.

Are you sure you need to ToList() in your question?

Newcomers to Linq often call ToList() a lot partly to keep things in a structure they can picture better, and partly because a lot of tutorial examples will make heavy use of it.

There are certainly times when ToList() is the only sensible thing to do, and also times when it is necessary to bring something from a non-in-memory form of Linq (such as against a database or XMLDocument) to in-memory. However:

If something starts in the database, then most of the time it's best to keep it there as long as possible. There are plenty of exceptions to this, but generally look to keep things in the database and optimise by bringing it into memory as an optimisation for those few exceptions, rather than being in the habit of bringing things into memory quickly and then optimising for the 97% of the time when keeping it in the database is faster!

If you need to switch to linq-to-objects, ToEnumerable() will do this without eagerly executing the query, so it's better than ToList() most of the time.

There are times when ToList() is the most efficient (especially if you have to hit the same list twice), but you should be calling it when you can see the clear need for it, not by default.