Calculating totals of a transaction

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 items, List coupons, List 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())
{
// 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);
}

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.

StackExchange Code Review Q#438, answer score: 12