Calculating business logic

Question

Here's a messy C# method used to calculate some business logic.

Does anybody have any suggestions on how I can optimise this code? I'm asking in case there are bad practices, improvements or functions within .NET/LINQ which I'm missing.

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

There's two things that I'm seeing right off the bat.

1. Don't use "magic integers" where you can be using Enums. GetDiscounts should probably receive an enum, not an int:

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

2. You don't need to Select then Max. Just using Max will give you the desired outcome.

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

Answer 2

It is not bad. Here is my shot at it:

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

Try this. There are several areas that are hard to comment on due to the null-based nature of several of the variables you are utilizing. Also the static helper methods on the bottom need to be refactored/replaced to conform with the methods you already have.

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