Runtime: Proposal Max, Min for IComparable

[benaadams]

public static class Math
{
    public static T Min<T>(T val1, T val2) where T : IComparable<T>;
    public static T Min<T>(T val1, T val2, IComparer<T> comparer);

    public static T Max<T>(T val1, T val2) where T : IComparable<T>;
    public static T Max<T>(T val1, T val2, IComparer<T> comparer);

    // ref returning version?
    public static ref T Min<T>(ref T val1, ref T val2) where T : IComparable<T>; 
    public static ref T Max<T>(ref T val1, ref T val2) where T : IComparable<T>;
}

/cc @NickCraver @DamianEdwards @terrajobst

[terrajobst]

I'd say those should live on Comparer.

/cc @weshaggard @KrzysztofCwalina

[benaadams]

But that is in System.Collections.Generic which also seems not the right place for a double value compare?

[benaadams]

Happy to go with it though :)

[terrajobst]

That's where comparers live because the assumpion was that comparisons are mostly done for sorting.

[benaadams]

Updated, suppose would have to be a static method then as can't have static (non-this) extension methods

[NickCraver]

Note this proposal already exists but didn't get much love: https://github.com/dotnet/corefx/issues/1583#issuecomment-219195181

I'd happy submit a PR but my understanding was: API review is a must first (makes sense), and no point in adding noise before that.

[benaadams]

@NickCraver Was hanging of Math. and not updated to go somewhere else.

Your suggestion on adding a new class of type Compare is interesting as it could be shipped in a different package with a much wider compat.

What would be the namespace for that type?

[khellang]

I think you can cut the <T> on the methods. You already have the T from the comparer, no?

[benaadams]

Yeahish; though that would make it more verbose... I'm liking @NickCraver's suggestion more something like:

public static class Compare
{
    public static T Min<T>(T val1, T val2) where T : IComparable<T>;
    public static T Min<T>(T val1, T val2, IComparer<T> comparer) where T : IComparable<T>;
    public static T Max<T>(T val1, T val2) where T : IComparable<T>;
    public static T Max<T>(T val1, T val2, IComparer<T> comparer) where T : IComparable<T>;
}

So usage would be determined via argument rather than class argument e.g.

Animal aA, bA;
SwimmingPool aS, bS;
string a, b;

Animal smallA = Compare.Min(aA, bA);
SwimmingPool largePool = Compare.Max(aS, bS);
string firstString = Compare.Min(a, b);

[benaadams]

Updated proposal; think it would be Net Standard 1.0 level? (.NET Framework 2.0)

[benaadams]

Question byref and ref returned result, how would that work, should it work?

Can't overload byref with not byref can you?

[nguerrera]

IMHO, this only makes sense on Comparer if it uses the comparer instance which would mean eliminating the IComparable constraint and having to use Comparer<U>.Default.Max(u1, u2) to route through IComparable. While more flexible (works with non default comparer), it's less discoverable and effectively drops the convenience of type inference (need to give Comparer an explicit T). And then there's no reason to make this work only on comparers that inherit from Comparer so those semantics are best as an extension method of IComparer.

Furthermore, it is unusual to hang facilities off of comparers. Generally, they are passed to an overload to get non-default comparison. We might want such an overload here.

Perhaps a new static Comparable type that is akin to Linq's Enumerable. Then we could have symmetry between Enumerable.Max and Comparable.Max.

[nguerrera]

The new proposal (which came in while I neglected to refresh) addresses my concerns. :)

[benaadams]

Initially I saw it as a simple ternary operator implementation e.g.

public static T Min<T>(T val1, T val2) where T : IComparable<T>
{
    return val1.CompareTo(val2) < 0 ? val1 : val2;
}

[nguerrera]

One thing: the overloads that take a comparer should not constrain T.

[nguerrera]

Also, I believe the byref overloads would make this uncallable from VB.

[AlexGhiondea]

I like the API shape (without the ref return). Let's review this.

[JonHanna]

If it does depend on IComparer<T> it could be an extension on such. Calling comparer.Min(x, y) seems a sensible enough thing to ask of comparer.

[jnm2]

If it does depend on IComparer it could be an extension on such. Calling comparer.Min(x, y) seems a sensible enough thing to ask of comparer.

I'd be calling Comparer<T>.Default.Min and (comparer ?? Comparer<T>.Default).Min a lot.

[terrajobst]

We concluded that Comparer<T> isn't good type, along the line of thinking you guys already had:

1. Forces to pass in a T
2. Cannot constrain T to be IComparable<T>, thus means implementation to call the general Compare method, which isn't going to be user fast.

We shouldn't add a new type. From the existing ones, it seems System.Math makes indeed the most sense.

[pgolebiowski]

I don't really feel like comparing instances of custom types -- or even strings -- using a method within System.Math. I can't say it feels natural. How about adding this functionality in two places? Based on ideas of @benaadams and @JonHanna.

public static class Compare
{
    public static T Min<T>(T a, T b) where T : IComparable<T>
    public static T Max<T>(T a, T b) where T : IComparable<T>
}

public interface IComparer<T>
{
    public T Min(T a, T b)
    public T Max(T a, T b)
}

• Want to use the nice and simple Compare.Min(x, y)? Feels natural if the arguments can be compared to each other, so they implement the IComparable<T>.
• Having a custom IComparer<T>, it should be capable of computing comparer.Min or comparer.Max given two values, as @JonHanna noticed.

I think this API feels natural. What do you guys think?

[benaadams]

We shouldn't add a new type. From the existing ones, it seems System.Math makes indeed the most sense.

Moved to Math