CSharpFunctionalExtensions icon indicating copy to clipboard operation
CSharpFunctionalExtensions copied to clipboard

Maybe.TryFirst() performance side-effect

Open leubac opened this issue 2 years ago • 2 comments

This method is causing me some trouble when I'm using it inside a query (CQRS-wise) when source is an IQueryable<T>.

public static Maybe<T> TryFirst<T>(this IEnumerable<T> source)
{
    source = source as ICollection<T> ?? source.ToList();

    if (source.Any()) return Maybe<T>.From(source.First());

    return Maybe<T>.None;
}

Since IQueryable<T> is not an ICollection<T> the call to .ToList() causes the whole database table to be fetched instead of only the first element (this is an issue when the table is really big). I understand the goal of calling ToList() is to optimize the subsequent calls to Any() and First() but at the cost of a nasty performance side-effect.

As far as I understand, this method could even be removed (or marked deprecated) because it is doing nothing more than what is already possible by calling IEnumerable<T>.FirstOrDefault() coupled with the implicit cast operator from T to Maybe<T> ?

leubac avatar Dec 02 '22 09:12 leubac

I agree with you, I implemented an Async version of this specifically for IQueryable because I'm doing alot of EFCore work at the moment:

https://github.com/vkhorikov/CSharpFunctionalExtensions/issues/473

 public static async Task<Maybe<T>> TryFirstAsync<T>(this IQueryable<T> source, CancellationToken cancellationToken = default)
    {

        var firstOrNull = await source.FirstOrDefaultAsync(cancellationToken);

        if (firstOrNull == null)
        {
            return Maybe<T>.None;
        }
        
        return Maybe<T>.From(firstOrNull);
    }

I did this ^^


What I did not like about the above code was the (3) calls , when it could be done in (1) call like @leubac said.

All you need is a .FirstOrDefault(), then check for null, and if not null, return the value. If null, return the result.


@leubac - we can wait until one of the contributors comments, but honestly i'd just make a PR.

I opened an issue on mine because mine uses IQueryable to take advantage of the IQueryable async methods

Good catch on this one, I saw the code a few hours ago, but for some reason it did not 'click' in my head to refactor it

jeffward01 avatar Dec 02 '22 10:12 jeffward01

Note that the fix here is to use another extension method that works on top of IQueriable, like the one @jeffward01 suggested. Any method working on top of IEnumerable (with any implementation) will prompt EF Core to load the full data set into memory.

vkhorikov avatar Dec 13 '22 15:12 vkhorikov