计算业务逻辑

Question

下面是一个用于计算某些业务逻辑的混乱的C#方法。

有人对我如何优化这段代码有任何建议吗？我要问的是，如果.NET/LINQ中有一些不好的实践、改进或功能，那么我就错过了。

private static decimal GetRecurringCostEquiv(List<QuotationItemViewModel> items)
{

    var discVal = GetDiscounts(items, (int)RecordEnums.ProductClassification.Service);

    decimal? currentRecurringCosts = GetRecurringCost(items).Value;

    decimal? result = 0.00M;

    decimal highestContractTerm = items.Where(x => x.ProductTypeId == (int)RecordEnums.ProductClassification.Service).Select(y => y.RecurringTerm).Max() ?? 0;

    if ((discVal > 0) && (highestContractTerm > 0))
    {
        var calc = items.Where(x => x.ProductTypeId == (int)RecordEnums.ProductClassification.Service).Select(y => y.RecurringCostCore * y.Quantity);
        decimal total = calc.Sum() ?? 0;
        decimal totalOverTerm = total * highestContractTerm;
        decimal totalOverTermMinusDiscounts = totalOverTerm - discVal;
        result = totalOverTermMinusDiscounts / highestContractTerm;
    }
    else
    {
        result = currentRecurringCosts;
    }

    return result ?? 0.00M;
}

Answer 1

有两件事我马上就看到了。

1. 不要在可以使用Enum的地方使用“魔术整数”。GetDiscounts可能会收到一个枚举，而不是int：var discVal = GetDiscounts(items, RecordEnums.ProductClassification.Service);
2. 你不需要Select那么Max。只需使用Max就可以获得所需的结果。decimal highestContractTerm = items.Where(x => x.ProductTypeId == (int)RecordEnums.ProductClassification.Service).Max(y => y.RecurringTerm ?? 0;

Answer 2

这还不错。这是我的机会：

private static decimal GetRecurringCostEquiv(List<QuotationItemViewModel> items)
{
    var discVal = GetDiscounts(items, (int)RecordEnums.ProductClassification.Service);

    // This can be calculated using its own small function
    // Consider using SQL-like LINQ syntax and split it into multiple lines.        
    decimal highestContractTerm = items.Where(x => x.ProductTypeId == (int)RecordEnums.ProductClassification.Service).Select(y => y.RecurringTerm).Max() ?? 0;

    if (discVal <= 0 || highestContractTerm <= 0)
    {
        return GetCurrentRecurringCosts(items);
    }

    // The following 2 lines could be its own function.
    // Again, consider splitting LINQ into multiple lines.
    var calc = items.Where(x => x.ProductTypeId == (int)RecordEnums.ProductClassification.Service).Select(y => y.RecurringCostCore * y.Quantity);
    decimal total = calc.Sum() ?? 0;

    // Get rid of the totalOverTerm variable and bring its value into the second line
    decimal totalOverTerm = total * highestContractTerm;
    decimal totalOverTermMinusDiscounts = totalOverTerm - discVal;

    // Do this instead:
    // return Coalesce(totalOverTermMinusDiscounts / highestContractTerm);

    decimal? result = totalOverTermMinusDiscounts / highestContractTerm;
    return result ?? 0.00M;
}

// Consider using this function
public static decimal Coalesce(decimal? nullableValue, decimal valueIfNull = Decimal.Zero)
{
    if (nullableValue.HasValue)
    {
        return nullableValue.Value;
    }

    return valueIfNull;
}

Answer 3

尝尝这个。有几个领域是很难评论的，因为您正在使用的几个变量都是基于空的。此外，底部的静态助手方法需要重构/替换，以符合您已经拥有的方法。

    private static decimal GetRecurringCostEquiv(this List<QuotationItemViewModel> items)
    {

        #region Variable Acquisition

        var products = items.Where(q=> q.ProductTypeId == (int)RecordEnums.ProductClassification.Service);
        var highestContractTerm = products.Max(q => q.RecurringTerm).Normalize();
        var discounts = items.GetDiscounts();

        var hasContractTerm = highestContractTerm > 0;
        var hasDiscounts = discounts > 0;

        #endregion

        if (hasDiscounts && hasContractTerm)
        {
            decimal cost = products.Sum(q => q.RecurringCostCore * q.Quantity).Normalize();

            return ((cost * highestContractTerm) - discounts) / highestContractTerm;
        }
        else 
        {
            return items.GetRecurringCost().Normalize();
        }
    }


    private static decimal? GetRecurringCost(this List<QuotationItemViewModel> items)
    {//TODO: should be reviewed/refactored according to the base function
        return GetRecurringCost(items).Value;
    }

    private static int GetDiscounts(this List<QuotationItemViewModel> items)
    {//TODO: should be reviewed/refactored according to the base function
        var discounts = 0; //Do Stuff

        return discounts > 0 ? discounts : 0;//ensure discount value compliance
    }


    private static decimal Normalize(this decimal? value) { return (decimal)value; }

    private static decimal Normalize(this int? value) { return (int)value; }