Removing too many IF's from object

Question

Edit:

I should have mentioned that I wanted it to be more Object Oriented. And I don't think my code here is anywhere near OO and neither is using switches, is it?

OP:

First of all, in my below example I am working with dutch units, so the calculations might seem off, but you'll get the idea.

Basically, what I have is a grocery list with products. In my database I store prices in "price by piece" or "price by pound", for example. So in order for calculating the total price of each product, based on the amount selected, I am working with the below class.

Example:

In my grocerylist I have a few products, and behind that product is a text field and a dropdown. In the textfield I can enter the amount I want to have, and in the dropdown I select if it needs to be ounces, pounds, and so on. Based upon those values, and the initial price from my database (price per piece and so on), I can calculate the total price for each product.

class Calculation
{
    protected $price;
    protected $amount;
    protected $unit;

public function __construct($price, $amount, $unit)
{
    $this->price = $price;
    $this->amount = $amount;
    $this->unit = $unit;
}

public function calculate()
{
    if($this->unit === 'ounce')
    {
        return $this->formatOunce();
    }
    if($this->unit === 'pound')
    {
        return $this->formatPound();
    }
        return $this->formatOne();
}

public function formatOne()
{
    return $this->price * $this->amount / 100;
}

public function formatOunce()
{
    return $this->price / 1000 * $this->amount / 100;
}

public function formatPound()
{
    return $this->price / 1000 * 500 * $this->amount / 100;
}

}

The problem I have is this:

public function calculate()
{
    if($this->unit === 'ounce')
    {
        return $this->formatOunce();
    }
    if($this->unit === 'pound')
    {
        return $this->formatPound();
    }
        return $this->formatOne();
}

How would I change the above code in order for it to be good OO? Do I use a Repository or an Interface for that? Or can I do that within this particular class to keep it simple? I feel there is way too many IF's.

Answer 1

Two immediate changes I would suggest:

1. use class constants instead of hard coded string identifiers. The advantage is twofold: better auto completion support by the IDEs and no more typos.

2. use a switch() statement, it makes things more clear:

---

class Calculation
{
    const UNIT_WEIGHT_OUNCE = 'ounce';
    const UNIT_WEIGHT_POUND = 'pound';
    // ...

    protected $price;
    protected $amount;
    protected $unit;
    // ...

    public function calculate()
    {
        switch ($this->unit) {
            case self::UNIT_WEIGHT_OUNCE: 
                return $this->formatOunce();
            case self::UNIT_WEIGHT_POUND:
                return $this->formatPound();
            // ...
            default:
                return $this->formatOne();
        }
    }

    // ...
}

This also would allow to have a "catalog of computations" and a single method instead of several standalone methods doing the actual computation, since you can store the formulas inside an array or even a static class again...

---

Finally a more basic thing: I would want to discuss the architectural approach here:

Why did you chose to implement a class Calculation? This feels pretty counter intuitive to me... Wouldn't it be much more natural to implement something like a "PositionInShoppingCart" instead? So that objects of that kind can hold a product identifier, a base price, a volume/amount and so on? That would lead to a "ShoppingCart" Class in a natural manner... Objects of that type would hold a list of object of the first type.

Answer 2

Wouldn't you like to switch to switch?

For one, switch would be conceptually and logically the right way to go. You are switching values using one variable $this->unit. And I think it's really clean to look at. [ When to use If-else if-else over switch statments and vice versa ]

Secondly, it is faster [ Is "else if" faster than "switch() case"? ]. Though in your case, might probably not matter that much.

$res = NULL;
switch($this->unit)
{
    case 'ounce': $res = $this->formatOunce(); break;
    case 'pound': $res = $this->formatPound(); break;
    default: $res = $this->formatOne();
}
return $res;

Answer 3

A OO oriented approach (as requested after editing the question):

You make a base class that handles the output total(). It calls a protected method that does the calculation. calculate():

class Item
{
    protected $price;
    protected $amount;

    public function __construct($price, $amount)
    {
        $this->price = $price;
        $this->amount = $amount;
    }

    protected function calculate()
    {
        return $this->price * $this->amount;
    }

    public function total($format = true)
    {
        if ($format) {
            return number_format($this->calculate(), 2);
        }
        return $this->calculate();
    }
}

Now you can extend you base item with a Pound version of the item. The Pound version will override the calculate() method because the calculation is done differently.

class PoundItem extends Item
{
    protected function calculate($format = true)
    {
        return $this->price / 1000 * 500 * $this->amount / 100;
    }
}

To produce your objects you'll need either a constructor method or what is called a factory to produce them. Here is a factory class. It could just as well have been implemented on your basket class.

class ItemFactory
{
    static public function create($price, $amount, $type)
    {
        // this could be implemented in numerous ways
        // it could even just be method on your basket
        $class = $type . "Item";
        return new $class($price, $amount);
    }
}

Creating a new item of the Pound type:

$a = ItemFactory::create(49.99, 25, "Pound");

Since PoundItem is also an Item you can use the total() method. But since we've changed the implementation of calculate() it now calculates for pounds.

echo $a->total();

Answer 4

What your question is edging towards is replacing conditionals with polymorphism, whereby you split out the code from each of the conditionals and put them in their own class: the Ounce class knows how to calculate ounces and Pound class knowns how to calculate pounds.

As someone said previously you'll still need some sort of factory which decides if it's time to use the Pound or Ounce. That factory will look... well, exactly like you have at the moment (but likely abstracted away somewhere else).

Because of how simple the code is at the moment, I think any of these changes would be refactoring too early. What you have at the moment isn't complicated - understanding what the code does isn't going to slow someone down here. Once you start having more complex ways to work out the calculation for something, then looking at polymorphism would be a better fit then.

Similarly, I think the switch statement vs IF conditional recommendations aren't very helpful suggestions either. In this case they do not add anything to readability.