10
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you, @Emma. I have them use the Google Java Style Guide, which seems more up-to-date than the Oracle one and also because I worked at Google for many years. I'll share the former article with them. I also assign portions of Effective Java, which is the course textbook. \$\endgroup\$ – Ellen Spertus Oct 14 at 15:00
  • 2
    \$\begingroup\$ Also: Please, please, when discussing that code, always point out that using floating point for money just begs for trouble. \$\endgroup\$ – Bobby Oct 14 at 18:20
  • 2
    \$\begingroup\$ @Bobby, you are absolutely right. A later part of the assignment is converting the doubles to BigDecimal. Effective Java is my Bible, and that is Psalm -- I mean Item -- 60. \$\endgroup\$ – Ellen Spertus Oct 14 at 21:55
  • 1
    \$\begingroup\$ @Ellen To play devil's advocate: A double has 18 signifcant digits. Hence in reality BigDecimal only makes sense for very specific situations where you do a loooong list of calculations on some values and want to keep a specific property. For things like a simple price using a fixed point number will work just as well. In your use case using a double and rounding at the end to the actual significant digits (2 or maybe 3 for most use cases) will give exactly the same result. \$\endgroup\$ – Voo Oct 15 at 11:03
  • \$\begingroup\$ There is an argument that this question should be closed as Unclear What You're Asking. You don't provide a description of what the function is supposed to do. So we have to guess from inspecting the code. For example, if the quantity is 14, this code will set the price as 14 * .95 * itemPrice. Is that correct? Or should it be 13 * itemPrice? Or 12.95 * itemPrice? Note that a customer could achieve the second price simply by making two separate purchases. In the future, please provide a problem statement for what the code is supposed to solve. \$\endgroup\$ – mdfst13 Oct 18 at 4:02
8
\$\begingroup\$

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...

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Yes, there are problems with the logic of the discount. I may have the students improve that as well. What I really want people's opinions on, however, is how the constants should be named. \$\endgroup\$ – Ellen Spertus Oct 14 at 14:53
  • 1
    \$\begingroup\$ @EllenSpertus I've expanded my constants suggestions, however as with everything name based... it's very subjective and there's no 'right' answers...just obviously wrong ones \$\endgroup\$ – forsvarir Oct 14 at 16:01
  • \$\begingroup\$ I'm going to claim that using constants is obviously wrong, whatever you call them. \$\endgroup\$ – Useless Oct 15 at 12:25
  • \$\begingroup\$ @Useless wrong in general, or just in this case? Context obviously has an impact, but I think it's a lot easier to argue that a constant TEN=11 is obviously wrong than constants are always wrong. \$\endgroup\$ – forsvarir Oct 15 at 13:38
  • \$\begingroup\$ I think that SPECIAL_ONE_OFF_DISCOUNT_QUANTITY looks pretty suspicious. Obviously I'm importing my real-world expectation of store discounting behaviour into the question when I assume the values are likely to change, but I think when you know something's value and aren't sure how to name it - that's often a strong indicator it should be treated as data instead of inserted into the code at all. \$\endgroup\$ – Useless Oct 15 at 13:55
6
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Whilst isn't a bad approach, some of it isn't intuitive (does it make sense to ask a Discount to calculate how much something costs without applying a discount)? Consider if it would be better for the Discount class to actually model the discount. So, rather than calculating the price after applying the discount, it calculated the amount of any discount that needed to be applied to the final price. Consider isOneOffFor and calculateOneOffFor which seem tightly coupled, modelling a discount, you could roll them together into a calculateOneOffDiscount which might be better. \$\endgroup\$ – forsvarir Oct 15 at 9:27
5
\$\begingroup\$

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.
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ You're absolutely right that I forgot to make the method static. My question was really how to name the constants. \$\endgroup\$ – Ellen Spertus Oct 14 at 14:51
  • \$\begingroup\$ @EllenSpertus Just to be clear, while you are welcome to suggest areas for review, it is always open for answerers to critique any and all aspects of the code and ignore your suggestions. In this, Code Review is somewhat different from other SE sites. It's less about questions and answers and more about code and observations. \$\endgroup\$ – mdfst13 Oct 18 at 3:51
4
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ At first glance it looks like this might give unexpected results for 14, 15, 16... You get the discount for the first 13, however because you're only left with 1,2,3 etc which is less than the trigger level for the 6 discount, so you get charged the full amount for these. I like the approach, and this is pseudocode so it would be fixed during implementation but I think it demonstrates that there's a trade-off between the complexity of the implementation and likelihood of change. If discounts change frequently then the extra work is justified, but if they change rarely, then it may not be. \$\endgroup\$ – forsvarir Oct 15 at 13:46
  • \$\begingroup\$ You're absolutely right! I didn't try to replicate the existing logic precisely. I mean, that's partly because I assume it was mistaken in the first place (eg. for the multiple baker's dozens case), but I'm just guessing at the desired spec. You could also use the same OrderedDiscountList to find the lowest possible total price instead of just using the current greedy algorithm. \$\endgroup\$ – Useless Oct 15 at 13:52
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

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
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.