While refactoring I noticed the following pattern:

public function doSomething() {
    $aVariable = '';

    if (X) {
        $aVariable = 'x';
    } else {
        $aVariable = 'y';
    }

    if ($this->isSomething()) {
        // ...
    } else {
        $anotherVariable = $aVariable . 'z';
        // ...
    }
}

In this example the variable $aVariable is defined at the top of the method, but it is only used in the “else” case of the second “if” statement. So this means the definition of $aVariable happens too early, at a time when it is not clear whether $aVariable is even needed. To fix it, we can move the first “if” block to the “else” case of the second “if”:

public function doSomething() {
    if ($this->isSomething()) {
        // ...
    } else {
        $aVariable = '';

        if (X) {
            $aVariable = 'x';
        } else {
            $aVariable = 'y';
        }

        $anotherVariable = $aVariable . 'z';
        // ...
    }
}

That’s better. But we are not finished yet. If you look at the code, the definition of $aVariable is very dominant, and can distract the reader of the code from the method’s purpose. To improve it, we could use the ternary operator:

$aVariable = X ? 'x' : 'y';

Using the ternary operator is sometimes seen as bad practice (see for example the coding standard for CakePHP). So it is better to apply the Extract Method refactoring:

public function doSomething() {
    if ($this->isSomething()) {
        // ...
    } else {
        $anotherVariable = $this->getAVariable() . 'z';
        // ...
    }
}

private function getAVariable() {
   $aVariable = '';

    if (X) {
        $aVariable = 'x';
    } else {
        $aVariable = 'y';
    }

    return $aVariable;
}

With that, the “doSomething” method has become easier to read and understand.

Happy refactoring :)