Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Problem comparing items implementing IComparable

I am working on a extension method where it finds the min item by specific selector. Below the code

    public static T MinBy<T, K>(this IEnumerable<T> src, Func<T, K> selector) where K : struct, IComparable, IConvertible
    {
        var min = default(K);
        T minItem = default(T);
        foreach (var item in src)
        {
            var current = selector(item);
            if (current < min)
            {
                min = current;
                minItem = item;
            }
        }

        return minItem;

    }

It gives error Error Operator '<' cannot be applied to operands of type 'K' and 'K'. But i have specified the generic constraint K should be Struct and IComparable. I believe all the numeric data type can be satisfied with this.

Then how come this is a invalid operation.?

like image 501
RameshVel Avatar asked Feb 24 '11 06:02

RameshVel


2 Answers

IComparable doesn't (and can't) say anything about operators. You should be using:

if (current.CompareTo(min) < 0)

Operators are static, and only ever overloaded rather than overridden. You can't require operators within interfaces, and the presence of a method doesn't magically change what an operator will do. (For example, overriding Equals does not change how == behaves.)

You should also note that as your constraint only talks about the nongeneric IComparable interface, you're going to be boxing at every operation. I would suggest you change the constraint to IComparable<K> instead. (Or drop the constraint and just use Comparer<K>.Default as Marc suggested.)

Some other comments about your method:

  • If all of the key values are more than the default value for K (e.g. K=int and all the keys are positive) then you won't find an item
  • You may wish to have an overload which accepts a particular IComparare<K> (but only if you drop the comparable constraint)
  • There's no real need to constrain K to a value type. What if I wanted to find the person with the lexicographically earliest name?
  • If there are no elements, it will return the default value for T; to fit in with the rest of LINQ I would suggest throwing InvalidOperationException
  • I would suggest using TSource and TKey as the type parameters to be more consistent with LINQ

You may want to look at the MoreLINQ MinBy implementation as an alternative. (Looking over that again, I'm not sure it's a good idea for us to require that comparer is non-null; it should probably use the default comparer in the same way as normal LINQ does if the comparer is null.)

like image 116
Jon Skeet Avatar answered Oct 01 '22 08:10

Jon Skeet


IComparable doesn't provide operator support - you need to use current.CompareTo(min). Or better, use Comparer<T>.Default.Compare(current,min) - then you can drop the constraint and it'll handle nulls etc automatically, and it'll avoid boxing.

var comparer = Comparer<T>.Default;
...
// loop
    if(comparer.Compare(current, min) < 0) {...}
like image 33
Marc Gravell Avatar answered Oct 01 '22 07:10

Marc Gravell