Calculating user efficiency on a time period with Linq

Question

I have this expression:

    public static readonly Expression<Func<ApplicationUser, decimal>> EfficiencyPercentExpr = e =>
        (e.ExecutorOrders.Where(x => x.Added >= DbFunctions.AddDays(SqlFunctions.GetUtcDate(), -30)).Count(x => x.Status != Order.OrderStatus.Deleted) > 0)
        ?
        (
        e.ExecutorOrders.Where(x => x.Added >= DbFunctions.AddDays(SqlFunctions.GetUtcDate(), -30)).Count(x => x.Status != Order.OrderStatus.Deleted)
        /
        e.ExecutorOrders.Where(x => x.Added >= DbFunctions.AddDays(SqlFunctions.GetUtcDate(), -30)).Count()
        )
        :
        0
    ;

This expression calculates the user's efficiency by calculating the percentage of successful orders from a total of the last 30 days.

How can I make it more clear?

Update:

There is a second approach of writing the expression, but it generates more expensive SQL query.

    private static readonly Expression<Func<ApplicationUser, decimal>> efficiencyPercentExpr = e =>
        e.ExecutorOrders.Where(x => !x.IsNotConsidered && x.Added >= DbFunctions.AddDays(SqlFunctions.GetUtcDate(), -30))
        .GroupBy(x => 1, (k, g) => new { Count = g.Count(), CountUndeleted = g.Count(x => x.Status != Order.OrderStatus.Deleted) })
        .Select(x => x.CountUndeleted > 0 ? (decimal)x.CountUndeleted / (decimal)x.Count : 0).FirstOrDefault();

In the future, please refrain from adding code to the question after it has already been answered. Unfortunately, the new code has also been reviewed, so I'll leave it there. — 200_success
– 200_success, Commented Dec 31, 2016 at 16:44

RubberDuck · Accepted Answer · 2016-12-29 09:55:09Z

7

Don't one line things that shouldn't be one lined. Just because you can do something, doesn't mean that you should.
Extract the duplicated query into a method of it's own that returns an IQueryable

Don't query the database twice for the same count, execute the query and store it in a variable.

private IQueryable<foo> ExecutorOrdersInLast30Days(ApplicationUser e)
{
    return e.ExecutorOrders.Where(x => x.Added >= DbFunctions.AddDays(SqlFunctions.GetUtcDate(), -30));
}

private decimal EfficiencyPercent(ApplicationUser e)
{
    var nonDeletedOrders = ExecutorOrdersInLast30Days(e).Count(x => x.Status != Order.OrderStatus.Deleted);
    if ( nonDeletedOrders > 0)
    {
        return nonDeletedOrders / ExecutorOrdersInLast30Days(e).Count();
    }
    else
    {
        return 0;
    }
}

edited Dec 29, 2016 at 9:55

answered Dec 28, 2016 at 22:02

RubberDuck

31.2k6 gold badges74 silver badges177 bronze badges

1

\$\begingroup\$ I think there is one more optimization possible. To use the GroupBy with for the Status so that the ExecutorOrdersInLast30Days doesn't have to be queried twice in case there are some non-deleted-orders. \$\endgroup\$

t3chb0t
– t3chb0t

2016-12-29 14:35:00 +00:00
Commented Dec 29, 2016 at 14:35
\$\begingroup\$ Thanks for your reply, but if I want to use EfficiencyPercent in filter queries, I can't do it with a computed property. With Expression I can do it easily using libraries like Linq.Translations (damieng.com/blog/2009/06/24/…). This is the main reason why I use this expression. \$\endgroup\$

sDima
– sDima

2016-12-30 13:21:16 +00:00
Commented Dec 30, 2016 at 13:21

Add a comment |

Stack Exchange Network

Calculating user efficiency on a time period with Linq

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Calculating user efficiency on a time period with Linq

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions