dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

MinOrDefault / MaxOrDefault #55138

Open TonyValenti opened 3 years ago

TonyValenti commented 3 years ago

Background and Motivation

Currently the linq Min and Max functions throw if the sequence contains no elements. It would be nice to be able to provide a default value that is used instead.

Proposed API

var min = Sequence.MinOrDefault(0); var max = Sequence.MaxOrDefault(100);

Usage Examples

See Above

Alternative Designs

Right now I do nastiness like this:

var MaxOrDefault = (
from x in Sequence
order by x descending
select x
).FirstOrDefault(100); 

Risks

None that I can think of.

@Foxtrek64 - Is this something you think you could get added to the framework? I'm tagging you because you added the FirstOrDefault(...) and other methods that are in line with this request.

dotnet-issue-labeler[bot] commented 3 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

huoyaoyuan commented 3 years ago

You are missing the Proposed API section. It's actually the usage. And your alternative design isn't alternative or workaround - it would still work like that with your proposed API.

Currently there are two potentialities:

Overloading like Min or Max:

namespace System.Linq
{
    class Enumerable
    {
+        int MinOrDefault(this IEnumerable<int> source, int defaultValue);
+        long MinOrDefault(this IEnumerable<long> source, long defaultValue);
+        float MinOrDefault(this IEnumerable<float> source, float defaultValue);
        // and so on for every numeric type
    }
}

This will keep API consistency, but bloats a lot.

An alternative design is to utilize recently merged generic math API. And we should also update Min and Max to utilize it. Then we will get:

namespace System.Linq
{
    class Enumerable
    {
+        T Min(this IEnumerable<T> source) where T : IComparisonOperators<T, T>;
+        T MinOrDefault(this IEnumerable<T> source, T defaultValue) where T : IComparisonOperators<T, T>;
+        T Max(this IEnumerable<T> source) where T : IComparisonOperators<T, T>;
+        T MaxOrDefault(this IEnumerable<T> source, T defaultValue) where T : IComparisonOperators<T, T>;
    }
}
huoyaoyuan commented 3 years ago

Well, the existing generic overload uses Comparer<T>.Default. I'm not sure what's the design approach on this. I do prefer not to bloat the API with overloads.

KalleOlaviNiemitalo commented 3 years ago

I don't see why any T would implement IComparisonOperators<T, T> but not IComparable<T>, or why T would implement those as inconsistent with each other. So Comparer<T>.Default would be fine semantically and more compatible with existing types. Although I wonder if IComparisonOperators<T, T> perhaps enables more inlining.

I don't remember ever wanting MinOrDefault or MaxOrDefault; where would one use these? Rather, I think I could have used a method that takes an additional element as a separate parameter, e.g. TSource Max<TSource>(this IEnumerable<TSource> source, TSource additionalElement). I don't intend to propose adding that, though.

Foxtrek64 commented 3 years ago

If this is approved, I'd be happy to implement it.

That said, I agree with Kalle. This is a situation I've run into only once or twice and I think my first thought would be something like

var max = myList.Any() ? myList.Max() : -1;

I could be misunderstanding your use case for this though.

huoyaoyuan commented 3 years ago

I don't remember ever wanting MinOrDefault or MaxOrDefault

Well, there is a simple and straight workaround: source.DefaultIfEmpty(defaultValue).Max()

ghost commented 3 years ago

Tagging subscribers to this area: @eiriktsarpalis See info in area-owners.md if you want to be subscribed.

Issue Details
## Background and Motivation Currently the linq ```Min``` and ```Max``` functions throw if the sequence contains no elements. It would be nice to be able to provide a default value that is used instead. ## Proposed API var min = Sequence.MinOrDefault(0); var max = Sequence.MaxOrDefault(100); ## Usage Examples See Above ## Alternative Designs Right now I do nastiness like this: ``` var MaxOrDefault = ( from x in Sequence order by x descending select x ).FirstOrDefault(100); ``` ## Risks None that I can think of. @Foxtrek64 - Is this something you think you could get added to the framework? I'm tagging you because you added the FirstOrDefault(...) and other methods that are in line with this request.
Author: TonyValenti
Assignees: -
Labels: `api-suggestion`, `area-System.Linq`, `untriaged`
Milestone: -
Foxtrek64 commented 3 years ago

I don't see why any T would implement IComparisonOperators<T, T> but not IComparable<T>, or why T would implement those as inconsistent with each other. So Comparer<T>.Default would be fine semantically and more compatible with existing types. Although I wonder if IComparisonOperators<T, T> perhaps enables more inlining.

I don't remember ever wanting MinOrDefault or MaxOrDefault; where would one use these? Rather, I think I could have used a method that takes an additional element as a separate parameter, e.g. TSource Max<TSource>(this IEnumerable<TSource> source, TSource additionalElement). I don't intend to propose adding that, though.

First/Last/Single use generics

public static TSource FirstOrDefault<TSource>(this IEnumerable<TSource> source, TSource defaultValue);

I think we can leverage that plus IComparable<TSource> and call it good, which is probably how I would write it. If IComparisonOperator<T, T> adds any special features that may be leveraged here, I may use that here instead.

public static partial class Enumerable
{
    public static TSource Max<TSource>(this IEnumerable<TSource> source)
        where TSource : IComparable<TSource>
    {
        TSource? max = source.TryGetMax(out bool found);
        if (!found)
        {
            ThrowHelper.ThrowNoElementsException();
        }

        return max!;
    }

    public static TSource? MaxOrDefault<TSource>(this IEnumerable<TSource> source) 
        where TSource : IComparable<TSource>
        => source.TryGetMax(out _);

    public static TSource MaxOrDefault<TSource>(this IEnumerable<TSource> source, TSource defaultValue)
        where TSource : IComparable<TSource>
    {
        TSource? max = source.TryGetMax(out bool found);
        return found ? max! : defaultValue;
    }
}
YohDeadfall commented 3 years ago

I don't think than limiting TSource to any constraint makes sense anymore. Comparer<T>.Default.Compare(T, T) is devirtualized (see #39873), so codegen should be fine. It was an issue during first days of LINQ, during .NET Framework times, where there was a need in a quick implementation and the need was mostly in basic types, but not now days. Moreover, the last proposed API will require a proper null handling, not a big deal, but still this is already implemented by comparers itself.

In addition, I think that it should be possible to get Max and Min values by a different condition than the default comparer provides, or allow handling of types from external libraries which has no comparers and not comparable without writing any wrapper.

Therefore< I propose the following API:

public static partial class Enumerable
{
    public static TSource Max<TSource>(this IEnumerable<TSource> source, IComparer<TSource>? comparer = null)
    {
        TSource? max = source.TryGetMax(comparer, out bool found);
        if (!found)
        {
            ThrowHelper.ThrowNoElementsException();
        }

        return max!;
    }

    public static TSource? MaxOrDefault<TSource>(this IEnumerable<TSource> source, IComparer<TSource>? comparer = null)
        => source.TryGetMax(comparer, out _);

    public static TSource MaxOrDefault<TSource>(this IEnumerable<TSource> source, TSource defaultValue, IComparer<TSource>? comparer = null)
    {
        TSource? max = source.TryGetMax(comparer, out bool found);
        return found ? max! : defaultValue;
    }
}

It can be used instead of OrderBy(...) followed by First() or FirstByDefault().