Practical PHP Refactoring: Replace Nested Conditionals with Guard Clauses

We agree that polymorphism eliminates many cases where if() statements are necessary. However, if we have a tangled conditional, it's better to simplify it as much as possible with local interventions before trying to extract new objects.

An example of tangled conditionals is actually a group of nested ones:

if ($user->isAdmin()) {
    if ($this->closed) {
        $displayed = "Closed (not for you)";
    } else if ($this->inEvidence) {
        $displayed = "Open and in evidence";
    } else {
        $displayed = "";
    }
} else {
    if ($this->closed) {
        $displayed = "Closed";
    } else {
        $displayed = "";
    }
}

How does code get this bad? One line at the time. It is a classical example of code where each line is added on a different day, and no one tries to improve it. If you throw a frog in boiling water, it will jump away; if you put it in fresh water and slowly heat it up, you get a frog soup.

The alternative

The alternative is to locally extract corner cases, in the form of guard clauses. In this kind of conditionals, one leg is an unusual case (the then block), while the other is the main case (which peforms a simple return, assuming the conditional is isolated in a method.)

Guard clauses scale better for long conditionals, since you can put many guard clauses that cover many corner cases at the start and then go on as normal.
Their proliferation however causes problems like for other conditionals, but if you devote a test to each corner case you cover all the guard clauses. The job of guard clauses is to simplify problematic code where a jungle of if/else/elseif would be the common result; if they are kept in leaf classes they cannot be duplicated in different places of the codebase.

Refactoring to guard clauses is another instance of the demise of the one entry, one exit rule.

Steps

A preliminary step requires the conditional to be isolated in a method whose return value should be the only thing that matters about the conditional execution. Eliminate side-effects or other assignments where possible. Then start the iterative process.

1. For each corner case, put a guard clause at the start of the method which performs an early return with the correct result. The tests should still pass.
2. Check the tests. The goal is to continue to have a green test, while eliminating parts of the code to refactor; the guard clauses make it so that the problematic cases do not reach the main part of the conditionals jungle.

The last case to cover is the main one, which should be out of conditionals.

Example

In the initial state, we have a little jungle of code.

<?php
class ReplaceNestedConditionalWithGuardClauses extends PHPUnit_Framework_TestCase
{
    public function testTheOpenTopicTitleIsDisplayedNormally()
    {
         $topic = new Topic("Hello");
         $this->assertEquals("Hello", $topic->__toString());
    }

    public function testTheClosedTopicTitleIsDisplayedWithACorrespondingIndication()
    {
         $topic = new Topic("Hello", true);
         $this->assertEquals("Closed: Hello", $topic->__toString());
    }

    public function testTheClosedTopicTitleIsDisplayedNormallyToAdmins()
    {
         $topic = new Topic("Hello", true, true);
         $this->assertEquals("Closed (not for you): Hello", $topic->__toString());
    }
}

class Topic
{
    private $title;
    private $isClosed;
    private $isAdminViewing;

    public function __construct($title, $isClosed = false, $isAdminViewing = false)
    {
        $this->title = $title;
        $this->isClosed = $isClosed;
        $this->isAdminViewing = $isAdminViewing;
    }

    public function __toString()
    {
        if (!$this->isClosed) {
            $displayed = $this->title;
        } else {
            if ($this->isAdminViewing) {
                $displayed = "Closed (not for you): $this->title";
            } else {
                $displayed = "Closed: $this->title";
            }
        }
        return $displayed;
    }
}

We add a first guard clause.

    public function __toString()
    {
        if ($this->isClosed && $this->isAdminViewing) {
            return "Closed (not for you): $this->title";
        }
        if (!$this->isClosed) {
            $displayed = $this->title;
        } else {
            if ($this->isAdminViewing) {
            } else {
                $displayed = "Closed: $this->title";
            }
        }
        return $displayed;
    }

We add the second and last guard clause. That's all, since we have exactly two tests more than the main one.

    public function __toString()
    {
        if ($this->isClosed && $this->isAdminViewing) {
            return "Closed (not for you): $this->title";
        }
        if ($this->isClosed) {
            return "Closed: $this->title";
        }
        if (!$this->isClosed) {
            $displayed = $this->title;
        } else {
            if ($this->isAdminViewing) {
            } else {
            }
        }
        return $displayed;
    }

We add the main case as an unconditional return statement.

    public function __toString()
    {
        if ($this->isClosed && $this->isAdminViewing) {
            return "Closed (not for you): $this->title";
        }
        if ($this->isClosed) {
            return "Closed: $this->title";
        }
        return $this->title;

        if (!$this->isClosed) {
            $displayed = $this->title;
        } else {
            if ($this->isAdminViewing) {
            } else {
            }
        }
        return $displayed;
    }

Now we can simplify the code, since a part of it is unreachable.

    public function __toString()
    {
        if ($this->isClosed && $this->isAdminViewing) {
            return "Closed (not for you): $this->title";
        }
        if ($this->isClosed) {
            return "Closed: $this->title";
        }
        return $this->title;
    }

Now we go on and try a polymorphic solution: to eliminate one of the two axis of change (the closed vs. open topic), we introduce a TopicState object.

Note that the code is broken up into different, smaller objects. I implement one of them with a guard clause too, the last if() that remains.

<?php
class ReplaceNestedConditionalWithGuardClauses extends PHPUnit_Framework_TestCase
{
    public function testTheOpenTopicTitleIsDisplayedNormally()
    {
         $topic = new Topic("Hello", new OpenTopicState);
         $this->assertEquals("Hello", $topic->__toString());
    }

    public function testTheClosedTopicTitleIsDisplayedWithACorrespondingIndication()
    {
         $topic = new Topic("Hello", new ClosedTopicState);
         $this->assertEquals("Closed: Hello", $topic->__toString());
    }

    public function testTheClosedTopicTitleIsDisplayedNormallyToAdmins()
    {
         $topic = new Topic("Hello", new ClosedTopicState, true);
         $this->assertEquals("Closed (not for you): Hello", $topic->__toString());
    }
}

class Topic
{
    private $title;
    private $isClosed;
    private $isAdminViewing;

    public function __construct($title, TopicState $isClosed, $isAdminViewing = false)
    {
        $this->title = $title;
        $this->isClosed = $isClosed;
        $this->isAdminViewing = $isAdminViewing;
    }

    public function __toString()
    {
        return $this->isClosed->topicCaption($this->isAdminViewing)
             . $this->title;
    }
}

interface TopicState
{
    /**
     * @return string
     */
    function topicCaption($isAdminViewing);
}

class OpenTopicState implements TopicState
{
    function topicCaption($isAdminViewing)
    {
        return '';
    }
}

class ClosedTopicState implements TopicState
{
    function topicCaption($isAdminViewing)
    {
        if ($isAdminViewing) {
            return 'Closed (not for you): ';
        }
        return 'Closed: ';
    }
}

We also rename the field to reflect the class name, to complete the operation.

class Topic
{
    private $title;
    private $topicState;
    private $isAdminViewing;

    public function __construct($title, TopicState $topicState, $isAdminViewing = false)
    {
        $this->title = $title;
        $this->topicState = $topicState;
        $this->isAdminViewing = $isAdminViewing;
    }

    public function __toString()
    {
        return $this->topicState->topicCaption($this->isAdminViewing)
             . $this->title;
    }
}