Practical PHP Refactoring: Remove Control Flag

Control flags are boolean variables used to determine if you should stop a cycle or go on for another iteration. Since iteration is one of the three basic operations of computing, they were pretty popular in C-based languages.

However, these flags are a relic from the times when the old maxim one entry point, one exit point was mandated as one of the rules of clean code. Even Fowler in his 1999 book says it's outdated. A single entry point (the method's signature) is good as it avoid gotos, but one exit point is a limitation. Sometimes logic is better expressed with an additional exit point.

You perfectly know this. But sometimes you inherit code and you have to improve it without introducing bugs and nuclear explosions.

Why eliminating a control flag?

A control flag is by definition actually used only in the last iteration of the cycle, so it is confusing for the majority of time. In ordinary iterations it is just ignored.

while ($i < $limit && !$elementFound) {
    ...
    if (...) {
        $elementFound == true;
    }
}

An equivalent situation features the flag not as a variable but as an additional condition:

for ($i = 0; $i < $limit && $array[$i] != $valueToSearch; $i++) {
}

Alternatives

Instead of control flag, you can use a special, universally recognized instruction to force the cycle to take a particular path. You can then delete the variable or the additional condition.

These instructions are available in all C-derived languages, including PHP:

• a break instruction interrupts a cycle, and makes a little jump to the line after its end.
• A continue instruction interrupts the current iteration of the cycle, and goes on to reevaluating the condition. Most of the time it starts the next iteration (like in for and foreach cycles).
• An early return instruction exits from the method, and gives (actually *returns*) the value passed to it as result. You can also just write *return;* to just terminate the method, in case its type is void.

Steps

1. Find the assignment(s) to the value that ends the cycle.
2. Substitute the assignments with a break statement, or a continue one.
3. Simplify the code by deleting the additional machinery that was making the flag work: the variables, the type of the cycle, else blocks that now can be part of the main cycle block.

Use a return when you want to exit from the whole method; if you extract a method containing a cycle, you will then use return in place of the break. Return is more common and easily understood than break, so it's preferrable.

Example

In the initial state, we are already following the Tell, don't Ask principle with the Users object, a first-class collection; it is a good start to avoid disseminating the same if() in all the client code that access a Users collection.

The test passes and I want to keep it green after each step.

<?php
class RemoveControlFlag extends PHPUnit_Framework_TestCase
{
    public function testFindsTheUserWhoseNameIsShortEnough() {
        $users = new Users(array('Giorgio', 'John', 'Tim'));
        $user = $users->findUserWithNameAsShortAs(3);
        $this->assertEquals('Tim', $user);
    }
}

class Users
{
    private $users;

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

    public function findUserWithNameAsShortAs($length)
    {
        $found = false;
        $i = 0;
        $length = count($this->users);
        while ($i < $length && !$found) {
            $user = $this->users[$i];
            if (strlen($user) == 3) {
                $found = true;
            }
            $i++;
        }
        return $user;
    }
}

We could outsource the if into a library function. Remember, the less conditionals, the fewer paths you have to test. However, you do not always has this option: this is a search, but the next piece of ugly code may be an incremental calculation of a value, or any other kind of computation.

So let's start by using a break instead of this flag. Wait, the break would jump at the end of the method; we can just use a return since in this scenario they're equivalent.

class Users
{
    private $users;

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

    public function findUserWithNameAsShortAs($length)
    {
        $found = false;
        $i = 0;
        $length = count($this->users);
        while ($i < $length && !$found) {
            $user = $this->users[$i];
            if (strlen($user) == 3) {
                $found = true;
                return $user;
            }
            $i++;
        }
    }
}

We do not need the flag anymore:

class Users
{
    private $users;

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

    public function findUserWithNameAsShortAs($length)
    {
        $i = 0;
        $length = count($this->users);
        while ($i < $length) {
            $user = $this->users[$i];
            if (strlen($user) == 3) {
                return $user;
            }
            $i++;
        }
    }
}

But now the while will always span the whole array (and return null) unless an early return is performed. So we can use a simple foreach and eliminate $i and $length.

<?php
class RemoveControlFlag extends PHPUnit_Framework_TestCase
{
    public function testFindsTheUserWhoseNameIsShortEnough() {
        $users = new Users(array('Giorgio', 'John', 'Tim'));
        $user = $users->findUserWithNameAsShortAs(3);
        $this->assertEquals('Tim', $user);
    }
}

class Users
{
    private $users;

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

    public function findUserWithNameAsShortAs($length)
    {
        foreach ($this->users as $user) {
            if (strlen($user) == 3) {
                return $user;
            }
        }
    }
}

Only now I recognize that I have hardcoded 3 as the length. Moreover, I had a clash in names as I was ignoring the $length input which now is gone. Another test would have told me that, but I can just write the obvious implementation now (it wasn't obvious before).

class Users
{
    private $users;

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

    public function findUserWithNameAsShortAs($length)
    {
        foreach ($this->users as $user) {
            if (strlen($user) == $length) {
                return $user;
            }
        }
    }
}

Note that given the requirements of a linear search, I would have written just this code, and I bet many of you would have done the same. However, refactorings are not always oriented to a short red-green-refactor cycle, but also to larger refactorings on code that has evolved to look ugly. For example, the while() we started from may had been followed by additional code some days ago, or had some other conditions for exiting...