Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maybe.TryFirst() performance side-effect #474

Open
leubac opened this issue Dec 2, 2022 · 2 comments
Open

Maybe.TryFirst() performance side-effect #474

leubac opened this issue Dec 2, 2022 · 2 comments

Comments

@leubac
Copy link

leubac commented Dec 2, 2022

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> ?

@jeffward01
Copy link

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:

#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

@vkhorikov
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants