HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Calculating user efficiency on a time period with Linq

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
linqperiodwithusertimecalculatingefficiency

Problem

I have this expression:

public static readonly Expression> 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> 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();

Solution


  • 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 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;
    }
}

Code Snippets

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;
    }
}

Context

StackExchange Code Review Q#151088, answer score: 7

Revisions (0)

No revisions yet.