Computing quantity-based discounts

Question

I'm a CS professor and provided this code as a small piece of a refactoring assignment:

private double getTotalPrice(int quantity, double itemPrice) {
    if (quantity == 13) {
        return itemPrice * 12; // Baker's dozen
    }
    else if (quantity >= 6) {
        return itemPrice * quantity * .95;
    } else {
        return itemPrice * quantity;
    }
}

Students are supposed to remove inappropriate magic numbers, but, when discussing the above code with them, I realized I don't know the best way to improve the above code. I'm not interested in improving performance, just teaching how to write clean and maintainable code.

Updates

I realize there are other problems with the code, some intentional. What I am most interested in is how to name the magic numbers.

The purpose of the code is to determine the total price for an order of up to 13 items (limit in place to prevent hoarding). My priorities are readability and maintainability.

Answer 1

Refactoring

The first thing I'd do is remove the redundant else statements. If you return from every branch, then there's really no need for the else clauses, they're just adding noise.

Constants?

It seems like there's some scope for constants / or lookups since there's some numbers floating around that seem to have meaning (discount amount, discount trigger threshold) etc. If you're labelling a number with a comment (baker's dozen), it's a good indication that it could be replaced with a constant to 'name' the value.

What are you really be looking for?

However, for me, I think the code actually serves more to prompt questions about whether or not it is performing as desired. If it is performing as expected, then it's the type of pricing system I hate (because it relies on the customer to optimize their own purchases).

If I buy 12 items, I'm charged for 12 at the discounted rate.

If I buy 13, I'm charged for 12 items at the standard rate.

If I buy 14, I'm charged for 14 at the discounted rate. This is more expensive than me buying 13, then buying a single item as a separate transaction.

Is this correct? Are you hitting me with the postage? Why don't I get 26 for the price of 24?

etc...

Constants Revisited

I think some of the above contribute to your difficulty coming up with constants... I'd go with something like this:

private static double getTotalPrice(int quantity, double itemPrice) {
    if (quantity == SPECIAL_ONE_OFF_DISCOUNT_QUANTITY) {
        return itemPrice * (SPECIAL_ONE_OFF_DISCOUNT_QUANTITY - 1);
    }
    if (quantity >= TRIGGER_DISCOUNT_QUANTITY) {
        return itemPrice * quantity * DISCOUNT_MULTIPLIER;
    }
    return itemPrice * quantity;
}

I'm using 'SPECIAL_ONE_OFF_DISCOUNT_QUANTITY', because it really is a special one off amount, with the current logic. It doesn't duplicate at 26 etc. I've got rid of the '12' constant, because really it's just 1 less than the trigger amount. Having a 'ONE_OFF_DISCOUNTTED_QUANTITY' constant might make sense if you could see the need to pay for 10 when you bought 13 for example, however this seems a it unnecessary.

As I said, I'd prefer to rework the logic so that you always pay the lowest amount + so that discounts are additive. This changes the output prices, so obviously wouldn't be possible if it didn't actually make sense to the client. It does allow some of the constants to have different names though:

private static double getTotalPrice(int quantity, double itemPrice) {
    var chargeableQuantity = quantity - quantity / BUY_X_GET_ONE_FREE_QUANTITY;
    var valueMultiplier = chargeableQuantity >= TRIGGER_DISCOUNT_QUANTITY ? DISCOUNT_MULTIPLIER : 1;

    return itemPrice * chargeableQuantity * valueMultiplier;
}

I think 'BUY_X_GET_ONE_FREE_QUANTITY' is a better name than 'SPECIAL_ONE_OFF_DISCOUNT_QUANTITY', but it only really makes sense if buying twice the amount results in twice the discount...

Answer 2

Another possibility would be an object-oriented approach in which we use the concept of abstraction.

For this we could abstract the logic quantity == 13 as a method with the sigiture isOneOffDiscountFor(int quantity):

private boolean isOneOffDiscountFor(int quantity) {
    return quantity == 13;
}

private boolean isCheaperDiscountFor(int quantity) {
    return quantity >= 6;
}

private double calculateOneOffDiscountFor(double price) {
    return price * 12;
}

private double calculateCheaperDiscountFor(double price, int quantity) {
    return calculateWithoutDiscount(price, quantity) * .95
}

private double calculateWithoutDiscount(double price, int quantity) {
    return price * quantity
}

private double getTotalPrice(int quantity, double itemPrice) {
    if (isOneOffDiscountFor(quantity)) {
        return calculateOneOffDiscountFor(itemPrice); // Baker's dozen
    }
    
    if (isCheaperDiscountFor(quantity)) {
        return calculateCheaperDiscountFor(itemPrice, quantity);
    }

    return calculateWithoutDiscount(itemPrice, quantity);
}

---

We can go further, because now we have included the word Discount everywhere in the newly added method names - which is an indication that we can encapsulate these methods in a separate object. That leads us to the next basic principle of object-oriented programming: encapsulation

class Discount {
  
    public boolean isOneOffFor(int quantity) {
        return quantity == 13;
    }

    public boolean isCheaperFor(int quantity) {
        return quantity >= 6;
    }

    public double calculateOneOffFor(double price) {
        return price * 12;
    }

    public double calculateCheaperFor(double price, int quantity) {
        return calculateWithout(price, quantity) * .95
    }

    public double calculateWithout(double price, int quantity) {
        return price * quantity
    }

}

private final Discount discount;

private double getTotalPrice(int quantity, double itemPrice) {
    if (discount.isOneOffFor(quantity)) {
        return discount.calculateOneOffFor(itemPrice);
    }
    
    if (discount.isCheaperFor(quantity)) {
        return discount.calculateCheaperFor(itemPrice, quantity);
    }

    return discount.calculateWithout(itemPrice, quantity);
}

An advantage of this variant is that through the encapsulation we achieve that the object more closely follow the Single Responsibility Principle, which in turn would make our code easier to test and more maintainable.

We can make the discount dynamic if a constructor is added to the Discount that makes the discount-values variable.

---

The self-drawn pictures are taken from my Github Repository about the Principles of Objekt-Oriented-Programming, in which the principle of inheritance and polymorphism are also visualized.

Answer 3

Other points:

• This is Java, so it's important that your method be marked static given that it is forced to be a class member
• Make a temporary variable double subTotal = itemPrice * quantity; to avoid duplication of the multiplication on the bottom
• When you do factor out magic numbers, whether they belong as class members or locals depends on the rest of the class, which you haven't shown. If the rest of the class reuses these constants then they belong as static final members. Otherwise, just make them local final.

Answer 4

What I am most interested in is how to name the magic numbers.

Alternative opinion - none of the numbers should be in code in the first place. Every answer with named constants is just wrong. They should all be in data.

Assumptions:

1. pricing rules are all predicated only on quantity
2. pricing rules change more frequently than business logic
3. if you buy 26 of something, you should be charged for two baker's dozens (2 * 12 = 24), not for simply "at least six" (26 * 0.95 = 24.7)

Pseudocode:

double getTotalPrice(int quantity, double itemPrice, OrderedDiscountList rules)
{
    double totalPrice = 0.0;
    int remaining = quantity;

    // require this is ordered with the largest quantity rule first
    for (rule in rules) {
        if (remaining >= rule.quantity) {
            int affected = remaining / rule.quantity;
            remaining = remaining % rule.quantity;
            totalPrice += affected * itemPrice * rule.multiplier;
        }
    }
    // so long as the last rule has quantity=1 and multiplier=1.0
    // we don't need to special-case anything in code
    return totalPrice;
}

configured with something like

OrderedDiscountList defaultRules =
{
    { .quantity = 13, .multiplier = 12.0/13 },
    { .quantity = 6,  .multiplier = 0.95 },
    { .quantity = 1,  .multiplier = 1.0 }
};

which could easily be read from JSON or similar when you want to update your price list without editing code.

NB. It's probably better to use a rational/ratio type than double for the multiplier, although I don't think we're hitting any rounding errors in this particular case.

Answer 5

One additional suggestion regarding the .95 constant:

In sales, it's more natural to express the discount percents instead of the factor

private static final double QUANTITY_DISCOUNT_PERCENT = 5.0;
private static final double QUANTITY_DISCOUNT_FACTOR = 0.01 * (100.0 - QUANTITY_DISCOUNT_PERCENT);
...
return itemPrice * quantity * QUANTITY_DISCOUNT_FACTOR;

(As already mentioned, using double arithmetic isn't a good idea...)

And (maybe it's overkill, but anyway):

Generalizing the two different discount approaches, this asks for a DiscountRule interface with at least two implementations: QuantityPercentDiscount and OneForFreeDiscount, an ordered list of discount rules, and code that chooses the first applicable rule for a given quantity.

Answer 6

For me, the best production code is the code that is concise and simple, especially with respect to the amount of branching. The following code slightly changes the semantics (in a good way, I think) and unifies the multiplication:

private double getTotalPrice(int quantity, double itemPrice) {
    // Charge for 13 as if it were only 12
    if (quantity == 13) quantity = 12;

    // Reduce item price for bulk purchases
    if (quantity >= 6) itemPrice *= 0.95;

    return quantity * itemPrice;
}

There are several good things to say about code like this.

• It's simple and easy to understand
• It's self-contained and doesn't depend on other functions/constants
• The comments make it clear what each step is doing, and the steps are applied in a clear linear order (which prompts the reader to consider what would happen if they were run in a different order--if the bulk discount threshold were also 13, it would make a difference...). This structure also helps avoid the pitfall in the original code where you don't get the bulk discount when you buy exactly 13 things!

Would this code really be improved by coming up with a new constant like BUY_X_GET_ONE_FREE_QUANTITY to stand in for 13? I would argue no, because that constant will probably live somewhere else in the program and will require an extra "hop" for a reader to check it. The net result is a bit more obfuscation of your code. It seems especially unproductive to hide this magic number inside a constant because most people will readily recognize 13 as the baker's dozen number.

Of course, in a real software system many other considerations might apply, such as:

• Do I need to use these numbers in other functions, such as perhaps the test suite? Maybe then I need static constants.
• Do I want to be able to configure these numbers like 13, 6, and 0.95 via a configuration system, or is it okay to bake them into the program? If it's the former, then many other considerations might apply, involving how the configuration is done and how powerful it needs to be.

FWIW, if I were free to overengineer this as much as I wanted, I would probably try to model it as a series of "promotions" which are applied via function composition. This would be more scalable and might work well at a bigger company, where different teams within the company might want to apply their own promotions to the pricing structure without stepping on each other's toes too much. For example: (in a pseudocode language because Java doesn't support tuples...)

// Each promotion may transform the quantity and/or item price
private [int, double] bakersDozenPromotion(int quantity, double itemPrice) {
    if (quantity == 13) return [12, itemPrice];
    else return [quantity, itemPrice];
}
private [int, double] bulkDiscountPromotion(int quantity, double itemPrice) {
    if (quantity >= 6) return [quantity, itemPrice * 0.95];
    else return [quantity, itemPrice];
}
// ...other promotions

private double getTotalPrice(int quantity, double itemPrice) {
    const allPromotions = compose(
        bakersDozenPromotion, 
        bulkDiscountPromotion, 
        ...
    )
    const [finalQuantity, finalItemPrice] = allPromotions(quantity, itemPrice);

    return finalQuantity * finalItemPrice;
}

By shifting the focus from "configuring individual constants" to "configuring individual promotion functions," we break the problem down more clearly into logical pieces. For example, in a real system we could imagine configuring our program by passing in a YAML file like the following, which even non-engineers at the company could edit:

promotions:
  bakers-dozen:
    enabled: true
    extra-free-unit-at: 13
  bulk-discount:
    enabled: true
    threshold: 6
    price-multiplier: 0.95