5
\$\begingroup\$

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();
\$\endgroup\$
7
\$\begingroup\$
  1. Don't one line things that shouldn't be one lined. Just because you can do something, doesn't mean that you should.
  2. Extract the duplicated query into a method of it's own that returns an IQueryable
  3. 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;
        }
    }
    
\$\endgroup\$
  • 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 Dec 29 '16 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 Dec 30 '16 at 13:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.