54

I am trying to use an IComparer to sort a list of Points. Here is the IComparer class:

public class CoordinatesBasedComparer : IComparer
{
    public int Compare(Object q, Object r)
    {
        Point a = (p)q;
        Point b = (p)r;
        if ((a.x == b.x) && (a.y == b.y))
            return 0;
        if ((a.x < b.x) || ((a.x == b.x) && (a.y < b.y)))
            return -1;

        return 1;
    }
}

In the client code, I am trying to using this class for sorting a list of points p (of type List<Point>):

CoordinatesBasedComparer c = new CoordinatesBasedComparer();
Points.Sort(c);

The code errors out. Apparently it is expecting IComparer<Point> as argument to sort method.
What do I need to do to fix this?

Jeff LaFay
  • 12,882
  • 13
  • 71
  • 101
Aadith Ramia
  • 10,005
  • 19
  • 67
  • 86

4 Answers4

68

You need to implement the strongly type interface (MSDN).

public class CoordinatesBasedComparer : IComparer<Point>
{
    public int Compare(Point a, Point b)
    {
        if ((a.x == b.x) && (a.y == b.y))
            return 0;
        if ((a.x < b.x) || ((a.x == b.x) && (a.y < b.y)))
            return -1;

        return 1;
    }
}

BTW, I think you use too many braces, I believe they should be used only when they contribute to the compiler. This is my version:

if (a.x == b.x && a.y == b.y)
    return 0;
if (a.x < b.x || (a.x == b.x && a.y < b.y))
    return -1;

Just like I dislike people using return (0).


Note that if you target a .Net-3.5+ application you can use LINQ which is easier and even faster with sorting.

LINQ vesion can be something like:

var orderedList = Points.OrderBy(point => point.x)
                        .ThenBy(point => point.y)
                        .ToList();
Uwe Keim
  • 39,551
  • 56
  • 175
  • 291
gdoron
  • 147,333
  • 58
  • 291
  • 367
  • When you say IComparer , doesn't Point stand as a placeholder variable value for which value has to be specified during instatiation? A little confused...if the class around which it works need not be specified at the client side, whats the point in having generic type? – Aadith Ramia Jan 15 '13 at 11:21
  • @Aadith, you got me confused... No, the interface: `IComparer` left you to chose with what of class you want to replace `T`, in your case you used `Point`. Just like when you create a Generic list of points: `List` which is `List` when T is Point. – gdoron Jan 15 '13 at 11:24
  • 1
    Be aware that the LINQ version is different from the non-LINQ version in that the `List` instance is replaced. If it is important that the list remains the *same* list, this should be noted. – O. R. Mapper Jan 15 '13 at 11:30
  • @O.R.Mapper. Just like with every LINQ method, with the fact that I never encountered a problem with it, I don't feel any need specifying it unless explicitly demanded to remain the original `List`. – gdoron Jan 15 '13 at 11:35
  • @gdoron got it..the point i was missing was that my class is actually a client to IComparer..thanks! – Aadith Ramia Jan 15 '13 at 12:58
  • 3
    >BTW, I think you use too many braces, I believe they should be used only when they contribute to the compiler. Braces help avoid mistakes. https://www.imperialviolet.org/2014/02/22/applebug.html – James South Jun 29 '20 at 20:08
  • Curly braces exist for a reason: https://blog.codecentric.de/en/2014/02/curly-braces/ – db2 Jan 27 '21 at 18:35
  • Nice post. But I disagree about braces. If at some point you need to addd a second statement to the true condition, it will break if you don't have the braces. I always use braces on if (and while etc.) statements, even if I put the result statement on the same line as the if like: if (a == b) {return 0;} – Mark Ainsworth Dec 16 '22 at 15:52
  • I took a few minutes to acquaint myself with LINQ OrderBy (thanks for the suggestion) and I like it WAY better than using IComparer. I had an array of Invoices (Invoice[] invoices) that I wanted to sort by a field called datePaid(newest first) Here is my Code: Invoice[] sortedInvoices = invoices.OrderByDescending(i => i.paidDate).ToArray(); One and done. I am a fan of strongly typed variables so I eschew var types. – Mark Ainsworth Dec 16 '22 at 17:08
14
public class CoordinatesBasedComparer : IComparer, IComparer<Point>
{
    public int Compare(Point a, Point b)
    {
        if ((a.x == b.x) && (a.y == b.y))
            return 0;
        if ((a.x < b.x) || ((a.x == b.x) && (a.y < b.y)))
            return -1;

        return 1;
    }
    int IComparer.Compare(Object q, Object r)
    {
        return Compare((Point)q, (Point)r);            
    }
}
Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • 2
    What's the point of implementing IComparer (the non-generic version) here, if all OP needs is the generic version? – zmbq Jan 15 '13 at 11:12
  • 4
    @zmbq a: because when writing a comparer, you don't *know* who the caller is, and what they need. And b: because it is 1 line of actual code, plus a method signature – Marc Gravell Jan 15 '13 at 11:14
  • +1, Only out of curiosity: Have you used `IComparer, IComparer` since LINQ entered the game?(in .Net3.5\C#3...) – gdoron Jan 15 '13 at 11:28
12

If you're slow like me, the -1 and 1 can be difficult to reason about when using IComparer. The way to think about it is when x should go first, return -1. When y should go first, return 1.

It can still get confusing if you have lots of fields to sort by. You can use an Enum to make your comparison logic more readable than 1 and -1, then cast the result.

This example puts objects with the least amount of null fields in the front.

public class NullishObjectsToTheBackOfTheLine: IComparer<ClassToCompare>
{
    private enum Xy
    {
        X = -1,
        Both = 0,
        Y = 1
    };

    //the IComparer implementation wraps your readable code in an int cast.
    public int Compare(ClassToCompare x, ClassToCompare y)
    {
        return (int) CompareXy(x, y);
    }

    private static Xy CompareXy(ClassToCompare x, ClassToCompare y)
    {
        if (x == null && y == null) return Xy.Both;

        //put any nulls at the end of the list
        if (x == null) return Xy.Y;
        if (y == null) return Xy.X;

        if (x.Country == y.Country && x.ProductId == y.ProductId) return Xy.Both;

        //put the least amount of at the front
        if (x.ProductId == null && x.Country == null) return Xy.Y;
        if (y.ProductId == null && y.Country == null) return Xy.X;

        //put the country values that are not nulls in front
        if (x.Country != y.Country) return x.Country != null ? Xy.X :  Xy.Y;

        //if we got this far, one of these has a null product id and the other doesn't
        return x.ProductId != null ? Xy.X : Xy.Y;
    }

}

public class ClassToCompare
{
    public string Country { get; set; }
    public string ProductId { get; set; }
}
Chad Hedgcock
  • 11,125
  • 3
  • 36
  • 44
  • This is wrong. 1 is for x > y and -1 when x < y. You can check it by calling CompareTo on int/double whatever. – bombek Jul 31 '21 at 12:17
  • 2
    Not wrong, that’s the same as saying “when x should go first, return -1. When y should go first, return 1.” – Chad Hedgcock Aug 01 '21 at 13:21
  • 1
    @ChadHedgock `If you're slow like me, the -1 and 1 can be difficult to reason about when using IComparer. The way to think about it is when x should go first, return -1. When y should go first, return 1.` . Mea culpa. The resutls are the same, but the journey follows different approach. Basicly what you do is sort from different side. Which I don't like. I just got mislead by this post. – bombek Aug 02 '21 at 10:51
0

I was getting an InvalidOperation error while adding an object of type MyClass to a SortedList<MyClass>. I was, incorrectly, implementing the IComparer interface. What I needed to implement was IComparable with the method CompareTo(MyClass other), instead of the ICompare.Compare(MyClass x, MyClass y). This is a simplified example:

SortedList<MyClass> sortedList = new SortedList<MyClass>();
MyClass a=new MyClass(), b=new MyClass();
sortedList.Add(a);
sortedList.Add(b); // Note, sort only happens once second element is added

This fixed it:

public class MyClass : IComparable<MyClass>
{
    int IComparable<MyClass>.CompareTo(MyClass other)
    {
        // DoCompareFunction(this, other); and return -1,0,1
    }
}

This was broken (don't do this if adding to SortedList<MyClass>):

public class MyClass : IComparer<MyClass>
{
    int IComparable<MyClass>.Compare(MyClass x, MyClass y)
    {
        // DoCompareFunction(x, y); and return -1,0,1
    }
}

This was the error:

Failed to compare two elements in the array.
at System.Collections.Generic.ArraySortHelper`1.BinarySearch(T[] array, Int32 index, Int32 length, T value, IComparer`1 comparer)
at System.Array.BinarySearch[T](T[] array, Int32 index, Int32 length, T value, IComparer`1 comparer)
at System.Collections.Generic.SortedList`2.Add(TKey key, TValue value)

Jason Hitchings
  • 667
  • 8
  • 10