Вычисление суммы сделки


Я работаю над проектом, где мне нужно рассчитать итоговые суммы сделки. К сожалению, купоны оказались довольно проблемой. Существует четыре вида купонов: сделки процент, суммы сделки, номенклатуры процент и товар суммы в долларах.

Я закончился вверх с этим зверем метода, и было интересно, если это ясно, почему я делаю каждую часть так, как я. Мои комментарии достаточно подробные? Это мой код более-менее читабельный? Есть ли способ упростить это?

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


4909
12
задан 30 января 2011 в 12:01 Источник Поделиться
Комментарии
4 ответа

Несколько замечаний:


  • Пожалуйста, избегайте комментариев, которые говорят вам "что" делает код. Нет никаких причин, чтобы есть комментарий, который говорит: "в следующей строке кода будет сложить два числа", когда я могу просто посмотреть на следующую строку и видим два числа добавляются. Комментарии должны быть использованы, чтобы объяснить "почему", а не "что".

  • Как правило, чистый код не должен требовать слишком много комментариев, кроме заголовков способ. Общий шаблон, чтобы смотреть на это, если у вас есть замечания следуют несколько строк кода, Вы, вероятно, следует считать, что блок кода и рефакторинг его в отдельный метод с соответствующим именем, с комментарием став заголовка метода.

  • Использовать вспомогательные методы. В CalculateTotals() метод должен быть всего несколько строк, который вызывает другие методы, например, CalculatePreDiscountSubtotal(), CalculateItemDiscounts(), CalculateTransactionDiscounts(), и CalculateStatistics(). Что делает его гораздо легче понять общий поток CalculateTotals(), в то же время делая все более мелкие куски проще для понимания и отладки.

  • Почему вы удаление элемента из списка, его изменения, а затем повторно добавить его? Вы должны быть в состоянии просто изменить объект в списке.

  • В петле пункт, вы действительно хотите сортировать элементы на каждой итерации? Не только есть перфорация оптимизация производится путем вытягивания из петли, логика здесь, возможно, сломаны. Вы собираетесь пересортировать предметы каждый раз со скидками, или по первоначальной цене? Другими словами, если у вас есть два купона, которые относятся к наиболее дорогой товар, вы хотите, чтобы они потенциально применимы к этому же пункту? Применяете ли вы их в одно время с тем, что товар является самым дорогим после того, как все купоны обработаны до сих пор? Или вы хотите, чтобы применить их на две самые дорогие вещи? Отметим, что во втором в последний вариант, порядок обработки купоны могло бы иметь значение.

  • Я бы порекомендовал не изменяя цену товара. Вместо этого, я хотел бы добавить, членом скидку на товар и шагом.

  • При расчете цены на номера-процентного купона, код будет потенциально сделать цену на элемент негативный, что, вероятно, не то, что вы хотите.

  • Почему не отследить скидку сделке отдельно, а не пытаться в равной степени вычесть его из каждого элемента в порядке? Это будет больше соответствовать реальной предметной области, и упростить код много. Не говоря уже о том, что код, как это может установить цену на товар отрицательное число.

12
ответ дан 30 января 2011 в 01:01 Источник Поделиться

Первое, что я заметил, что вы используете ?? 0 много. Если вы могли бы реструктурировать свой код, чтобы гарантировать, что цены не будут нулевыми , что позволит вам удалить те, которые хотели очистить ваш код немного.

Далее вы часто, используя шаблон:

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

Это может быть упрощен с помощью LINQ-это сумма способ:

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

(Или просто я.Цена вместо меня.Цена ?? 0 если вы последуете моему предыдущему предложению).

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

Если товар имеет тип значения, вам не нужно, чтобы удалить и повторно добавить его, чтобы изменить его.

8
ответ дан 30 января 2011 в 12:01 Источник Поделиться

Я бы отдельный купон Скидка логику в отдельный класс. Есть класс CouponBase, который имеет функцию .ApplyCouponToItem(элемент). Затем вы можете иметь класс PercentageCoupon и класс WholeAmountCoupon, и каждый может переопределить эту функцию, чтобы делать то, что он должен делать. Таким образом, ваш код не будет загроможден со скидкой логики и будет лучше отделить в лице будущих изменений купоны.

2
ответ дан 30 января 2011 в 03:01 Источник Поделиться

Большинство из них уже упоминались, но я бы хотел добавить:


  1. Избавиться от товара.Цена ?? 0. Похоже, Вы, наконец, пропустили эту конструкцию в TransactionCoupon петли для некурящих, процентных купонов.

    У вас есть:

    item.Price = item.Price - discount;

  2. Используйте сумму вместо оператора foreach. Это имеет два преимущества - работает на IEnumerable и позволяет более легко понять, что в этой конкретной линии нужно только сумму элементов на каждой итерации, а не то, что было в аккумуляторе перед.

  3. Не вводить всех вас переменные (если вы пишете на C) в начале метода. Обычно рекомендуется ввести переменную прямо перед его первым использованием. Это позволит вам присоединиться к переменной и это задание, что также улучшает читаемость. И в этом случае вы сможете использовать объект-инициализатор для общей переменной, которые (ИМО) лучше, чем прямое назначение свойства вы делаете в последних строках.

  4. Здесь слишком много скобок:

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

  5. Какой-либо причине взять параметры как список , а не возвращает?

2
ответ дан 30 января 2011 в 02:01 Источник Поделиться