Practical PHP Refactoring: Consolidate Duplicate Conditional Fragments

A piece of code is contained in all legs of a conditional: an obvious solution is to move it outside to simplify the branches. It may seem stupid to come up with code that is duplicate in some branches, but often it is a result of a process of transforming existing code: it is not written at once like this.

Moreover, often the duplication is not clearly visible, as there are other lines mixed up with the duplicated ones; or some assigments that complicate one copy of the same lines.

Why eliminating duplicate fragments?

This refactoring is preliminary to a possible polymorphism solution: the smaller the conditionals, the simpler is is to transport them in other objects.

By minimizing the conditional code, you get a great help in defining what has to change between the different objects to compose; just consider that each variable you eliminate from inside the {} of a conditional block is one reference that you do not have to pass to another object, either via a constructor or a method parameter.

Steps

Identify the duplicated code: it should be present inside all branches (then, else or elseif when present). You may have to add some local variables to break some lines into their duplicated and specific parts.

Then, the alternatives presented by Fowler are:

• The duplicated code is at the beginning of a conditional block: move it before the conditional.
• The duplicated code is situated at the end: move it after the conditional.
• The code is in the middle of other statements. You should try to move it forward or backward, if it does not functionally change the result. Often the position of a statement is not strict, as it shows an independence from the nearby statements.

Note that all the code eliminated from the conditionals can be extracted in a method when it's longer than a few lines.

Example

(I'm not using fixed precision numbers, since it is money I should but it's better to keep the example short)
Invoice: add a fixed processing fee before taxes, or if there is a discount wave this fee together with it

It's not immediately clear that there's some duplicate code here:

<?php
class ConsolidateDuplicateConditionalFragments extends PHPUnit_Framework_TestCase
{
    public function testTotalPaymentIncludeTaxesAndProcessingFee()
    {
        $invoice = new Invoice(990, 21, false);
        $this->assertEquals(1210, $invoice->getTotal());
    }

    public function testTotalCanBeDiscountedBeforeTaxes()
    {
        $invoice = new Invoice(1250, 21, 20);
        $this->assertEquals(1210, $invoice->getTotal());
    }
}

class Invoice
{
    private $taxable;
    private $taxRate;
    private $discount;
    const PROCESSING_FEE = 10;

    public function __construct($taxable, $taxRate, $discount = false)
    {
        $this->taxable = $taxable;
        $this->taxRate = $taxRate;
        $this->discount = $discount;
    }

    public function getTotal()
    {
        if ($this->discount) {
            $total = $this->taxable * (1 - $this->discount / 100);
            return $total * (1 + $this->taxRate / 100);
        } else {
            return ($this->taxable + self::PROCESSING_FEE) * (1 + $this->taxRate / 100);
        }
    }
}

But if we look at the logical steps, it should be: tax addition to the taxable amount should be unrelated to the presence of a discount. Let's rewrite it:

class Invoice
{
    private $taxable;
    private $taxRate;
    private $discount;
    const PROCESSING_FEE = 10;

    public function __construct($taxable, $taxRate, $discount = false)
    {
        $this->taxable = $taxable;
        $this->taxRate = $taxRate;
        $this->discount = $discount;
    }

    public function getTotal()
    {
        if ($this->discount) {
            $total = $this->taxable * (1 - $this->discount / 100);
            return $total * (1 + $this->taxRate / 100);
        } else {
            $total = $this->taxable + self::PROCESSING_FEE;
            return $total * (1 + $this->taxRate / 100);
        }
    }
}

Now we see that there is a duplication. If we were to refactor now to a polymorphic solution, we should take into consideration also how to compute the tax. Let's move the tax-related code out of the conditional.

class Invoice
{
    private $taxable;
    private $taxRate;
    private $discount;
    const PROCESSING_FEE = 10;

    public function __construct($taxable, $taxRate, $discount = false)
    {
        $this->taxable = $taxable;
        $this->taxRate = $taxRate;
        $this->discount = $discount;
    }

    public function getTotal()
    {
        if ($this->discount) {
            $total = $this->taxable * (1 - $this->discount / 100);
        } else {
            $total = $this->taxable + self::PROCESSING_FEE;
        }
        return $total * (1 + $this->taxRate / 100);
    }
}

The refactoring considered in this article is finished. However, I will go on in showing a simple polymorphic solution.

We introduce a Discount object, and move everything related to the conditional there (everything not related stays in the curent class, since we plan to make multiple implementation of discount)

<?php
class ConsolidateDuplicateConditionalFragments extends PHPUnit_Framework_TestCase
{
    public function testTotalPaymentIncludeTaxesAndProcessingFee()
    {
        $invoice = new Invoice(990, 21, new Discount(false));
        $this->assertEquals(1210, $invoice->getTotal());
    }

    public function testTotalCanBeDiscountedBeforeTaxes()
    {
        $invoice = new Invoice(1250, 21, new Discount(20));
        $this->assertEquals(1210, $invoice->getTotal());
    }
}

class Discount
{
    private $rate;
    const PROCESSING_FEE = 10;

    public function __construct($rate)
    {
        $this->rate = $rate;
    }

    public function discount($amount)
    {
        if ($this->rate) {
            return $amount * (1 - $this->rate / 100);
        } else {
            return $amount + self::PROCESSING_FEE;
        }
    }
}

class Invoice
{
    private $taxable;
    private $taxRate;
    private $discount;

    public function __construct($taxable, $taxRate, Discount $discount)
    {
        $this->taxable = $taxable;
        $this->taxRate = $taxRate;
        $this->discount = $discount;
    }

    public function getTotal()
    {
        $total = $this->discount->discount($this->taxable);
        return $total * (1 + $this->taxRate / 100);
    }
}

Now we have a small object with two different behaviors depending on its state. We should really divide the logic in two classes:

<?php
class ConsolidateDuplicateConditionalFragments extends PHPUnit_Framework_TestCase
{
    public function testTotalPaymentIncludeTaxesAndProcessingFee()
    {
        $invoice = new Invoice(990, 21, new ProcessingFee);
        $this->assertEquals(1210, $invoice->getTotal());
    }

    public function testTotalCanBeDiscountedBeforeTaxes()
    {
        $invoice = new Invoice(1250, 21, new PercentageDiscount(20));
        $this->assertEquals(1210, $invoice->getTotal());
    }
}

interface Discount
{
    public function discount($amount);
}

class PercentageDiscount implements Discount
{
    private $rate;

    public function __construct($rate)
    {
        $this->rate = $rate;
    }

    public function discount($amount)
    {
        return $amount * (1 - $this->rate / 100);
    }
}

class ProcessingFee implements Discount
{
    const PROCESSING_FEE = 10;

    public function discount($amount)
    {
        return $amount + self::PROCESSING_FEE;
    }
}

class Invoice
{
    private $taxable;
    private $taxRate;
    private $discount;

    public function __construct($taxable, $taxRate, Discount $discount)
    {
        $this->taxable = $taxable;
        $this->taxRate = $taxRate;
        $this->discount = $discount;
    }

    public function getTotal()
    {
        $total = $this->discount->discount($this->taxable);
        return $total * (1 + $this->taxRate / 100);
    }
}

Wait, there is a Discount now that actually adds money to the amount to be paid. We had better call it differently, both for the class and the method:

interface PaymentModifier
{
    public function applyOn($amount);
}

class PercentageDiscount implements PaymentModifier
{
    private $rate;

    public function __construct($rate)
    {
        $this->rate = $rate;
    }

    public function applyOn($amount)
    {
        return $amount * (1 - $this->rate / 100);
    }
}

class ProcessingFee implements PaymentModifier
{
    const PROCESSING_FEE = 10;

    public function applyOn($amount)
    {
        return $amount + self::PROCESSING_FEE;
    }
}

class Invoice
{
    private $taxable;
    private $taxRate;
    private $paymentModifier;

    public function __construct($taxable, $taxRate, PaymentModifier $paymentModifier)
    {
        $this->taxable = $taxable;
        $this->taxRate = $taxRate;
        $this->paymentModifier = $paymentModifier;
    }

    public function getTotal()
    {
        $total = $this->paymentModifier->applyOn($this->taxable);
        return $total * (1 + $this->taxRate / 100);
    }
}