Calculating totals of a transaction

Question

I am working on a project where I have to calculate the totals of a transaction. Unfortunately, coupons have proven to be quite an issue. There are four types of coupons: transaction percentage, transaction dollar amounts, item percentage and item dollar amounts.

I ended-up with this beast of a method, and was wondering if it is clear why I'm doing each part in the way I am. Are my comments are detailed enough? Is my code is more-or-less readable? Is there a way to simplify any of it?

private static Totals CalculateTotals(List<Item> items, List<Coupon> coupons, List<Payment> payments)
{
    // Local vars, totals gets returned.
    Totals totals = new Totals();
    decimal subtotal = 0;
    decimal workingSubtotal = 0;
    decimal discounts = 0;
    decimal tax = 0;
    decimal paid = 0;
    decimal taxRate = (Initialization.Location.TaxRate ?? 0);


    // Get the subtotal before any discounts.
    items.ForEach(i => subtotal += (i.Price ?? 0));


    // An ItemCoupon takes a whole amount or percentage off of a single Item.
    // It can take it off of the most expensive, or least.  Nothing in the middle.
    foreach (var coupon in coupons.OfType<ItemCoupon>())
    {
        // new Item to hold the item to be discounted.
        Item item;

        // Find which item to discount.
        if (coupon.DiscountMostExpensive)
        {
            item = items.OrderByDescending(i => i.Price).FirstOrDefault();
        }
        else // Otherwise, Discount LEAST Expensive.
        {
            item = items.OrderByDescending(i => i.Price).LastOrDefault();
        }

        // Remove it from the list, before editing the price.
        items.Remove(item);

        // Set new price of item based on the type of coupon. (Percent, or whole dollar.)
        if (coupon.PercentageCoupon)
        {
            item.Price = Utils.CalculatePercentage(item.Price, coupon.DiscountPercentage);
        }
        else
        {
            item.Price = (item.Price ?? 0) - (coupon.DiscountAmount ?? 0);
        }

        // Add the item back to the list, with the new price.
        items.Add(item);
    }


    // Now that the single items have been discounted, let's get a wroking subtotal.
    items.ForEach(i => workingSubtotal += (i.Price ?? 0));


    // A TransactionCoupon takes a whole amount or percentage off of the entire transaction. 
    // To simplfy tax caculation--and because some items are non-taxable, 
    // oh and, because we don't want any one item going below zero--we 
    // split the discount over all the items, evenly.
    foreach (var coupon in coupons.OfType<TransactionCoupon>())
    {
        if (coupon.PercentageCoupon)
        {
            // If it is a Percentage Coupon, simply take said percentage of off each item.
            items.ForEach(i => i.Price = Utils.CalculatePercentage(i.Price, coupon.DiscountPercentage));
        }
        else
        {
            // If it is a whole amount, get each items percent of the of the subtotal, and discount them equally.
            // This would look way too confusing using lambda.
            foreach (var item in items)
            {
                decimal discount = (item.Price ?? 0) * ((coupon.DiscountAmount ?? 0) / workingSubtotal);
                item.Price = item.Price - discount;
            }
        }
    }


    // Let's get the new-new-new subtotal.
    workingSubtotal = 0;
    items.ForEach(i => workingSubtotal += (i.Price ?? 0));


    // Calculate the total discounts.
    discounts += (subtotal - workingSubtotal);


    // Set tax for order.  (This must be done after ALL discounts have been applied)
    foreach (var item in items.Where(i => i.Taxable))
    {
        tax += ((item.Price ?? 0) * taxRate);
    }


    // Get the total amount paid.
    payments.ForEach(p => paid += p.Amount);


    // Add all the results to the Totals struct.
    totals.Subtotal = subtotal;  // Never return the workingSubtotal;
    totals.Discounts = discounts;
    totals.Tax = tax;
    totals.Paid = paid;
    totals.Total = ((workingSubtotal + tax) - paid);

    return totals;
}

Answer 1

A few comments:

• Please avoid comments that tell you "what" the code is doing. There is no reason to have a comment that says "The next line of code will add two numbers", when I can just look at the next line and see the two numbers being added. Comments should be used to explain "why", not "what".
• Generally, clean code shouldn't require too many comments other than method headers. A common pattern to look for is if you have a comment followed by several lines of code, you should probably take that block of code and refactor it into a separate method with a descriptive name, with the comment becoming the method header.
• Use helper methods. The CalculateTotals() method should be just a few lines that calls into other methods like CalculatePreDiscountSubtotal(), CalculateItemDiscounts(), CalculateTransactionDiscounts(), and CalculateStatistics(). That makes it much easier to understand the overall flow of CalculateTotals(), while also making each of the smaller pieces easier to understand and debug.
• Why are you removing item from the list, modifying it, and then re-adding it? You should be able to just modify the object while it's in the list.
• In the item loop, do you really want to sort the items on each iteration? Not only is there a perf optimization to be made by pulling it out of the loop, the logic here is possibly broken. Did you intend to re-sort the items every time with discounts applied, or by original price? In other words, if you have two coupons that apply to the most expensive item, do you want them to potentially apply to the same item? Do you apply them one at a time with whatever item is most expensive after all coupons processed so far? Or do you want to apply them to the two most expensive items? Note that in the second to last option, the order in which you process the coupons might matter.
• I would recommend not modifying the Price of an Item. Instead, I would add a Discount member to Item and increment that.
• When calculating the price for a non-percentage coupon, your code will potentially make the price of an item negative, which is probably not what you want.
• Why not keep track of the transaction discount separately rather than trying to equally subtract it from each item in the order? It would more closely match the actual problem domain, and simplify the code a great deal. Not to mention that the code as is can set the price of an item to a negative number.

Answer 2

The first thing that I noticed is that you're using ?? 0 a lot. If you could restructure your code to ensure that the prices won't be null that would allow you to remove those, which would clean up your code a bit.

Further you're often using the pattern:

int foo = 0;
//...
items.ForEach(i => foo += (i.Price ?? 0));

This can be simplified using LINQ's Sum method to:

int foo = items.Sum(i => i.Price ?? 0);

(Or just i.Price instead of i.Price ?? 0 if you heed my earlier suggestion).

// Remove it from the list, before editing the price.
items.Remove(item);

Unless item is a value-type, you don't need to delete and re-add it to change it.

Answer 3

I would seperate the coupon discount logic into a seperate class. Have a CouponBase class, that has a function .ApplyCouponToItem(Item item). Then you can have a PercentageCoupon class and a WholeAmountCoupon class, and each one can override that function to do what it needs to do. This way, your code won't be cluttered with discount logic and will be better decoupled in the face of future changes to coupons.

Answer 4

Most of these were already mentioned but I'd like to add:

1. Get rid of item.Price ?? 0. Looks like you finally missed this construction in TransactionCoupon loop for non-percentage coupons.

   You have there:

   item.Price = item.Price - discount;
   

2. Use Sum instead of ForEach. It has two advantages - works on IEnumerable and allows more easily understand that in this particular line you need only sum of items being iterated, but not something that was in accumulator before.

3. Do not introduce all you variables (unless you write in C) in the beginning of the methods. It is usually recommended to introduce variable right before it's first use. It will allow you to join variable declaration and it's assignment, which also improves readability. And in this case you will be able to use object-initializer for total variable, which (IMO) is better than direct property assignment you're doing in last lines.

4. Too many parentheses here:

   totals.Total = ((workingSubtotal + tax) - paid);
   

5. Any reason to take parameters as List instead of IEnumerable?