Why does clean code forbid else expression

Question

I have this code in a function:

if ($route !== null) { // a route was found
    $route->dispatch();
} else {
    // show 404 page
    $this->showErrorPage(404);
}

Now PHPmd gives an error:

The method run uses an else expression. Else is never necessary and you can simplify the code to work without else.

Now I'm wondering if it really would be better code to avoid the else and just add a return statement to the if part?

Controvert

Answer 1

I wouldn't worry about what PHPmd says , atleast in this case.

They probably meant for you to use the conditional operator because (in their opinion) its 'cleaner'.

$route !== null  ?  $route->dispatch() : $this->showErrorPage(404) ;

Cistern

So is there something better than phpMD? – Controvert

learning from phptherightway.com is often suggested by many programmers – Cistern

How is that cleaner? maybe at a return statement or something but other than these are exactly the opposite of clean code – Mindi

Finding ternary operator in a piece of code immediately suggests conditional assignment rather than flow control. Yes, it can be abused to do that, but doing so for the sake of clean code looks a bit forced. – Barney

Answer 2

PHPMD is expecting you to use an early return statement to avoid the else block. Something like the following.

function foo($access) 
{
    if ($access) {
        return true;
    }

    return false;
}

You can suppress this warning by adding the following to your class doc block.

/**
 * @SuppressWarnings(PHPMD.ElseExpression)
 */

Guerrero

Is there any paper or any kind of reference material one can take a look to try and understand this rule of no else? – Adabelle

@Adabelle you can find the rule details here: phpmd.org/rules/cleancode.html#elseexpression. PSR-2 recommendation on the same can be found here. github.com/php-fig-rectified/fig-rectified-standards/blob/… – Guerrero

The rules actually say to use Else statements. github.com/php-fig-rectified/fig-rectified-standards/blob/… – Adabelle

The rules and the PSR recommendation says else is not necessary. PHP MD Rule Set Documentation says: An if expression with an else branch is never necessary. You can rewrite the conditions in a way that the else is not necessary and the code becomes simpler to read. To achieve this use early return statements. PSR-2 Standard says in the write better code section: github.com/php-fig-rectified/fig-rectified-standards/blob/… * Try to return early in methods/functions to avoid unnecessary depths – Guerrero

Maybe we are reading different things but here is what I am reading "Ternary operators are permissible when the entire ternary operation fits on one line. Longer ternaries should be split into if else statements." Notice the ´split into if else statements`. Also multiple returns are not great – Adabelle

A famous book author is Robert C. Martin that wrote "Clean Code: A Handbook of Agile Software Craftsmanship". This rule is not a PHP invention. – Owl

Answer 3

You usually can rewrite the expression to use just an if and it does subjectively make the code more readable.

For example, this code will behave in the same way if showErrorPage breaks the execution of the code.

if ($route == null) { 

   $this->showErrorPage(404);
} 
$route->dispatch();

If the content of your if statement does not break the execution, you could add a return

if ($route == null) { 

   $this->showErrorPage(404);
   return;
} 
$route->dispatch();

If you where inside a loop, you could skip that iteration using continue

    foreach ($things as $thing ) {
        if ($thing == null) {
            //do stuff and skip loop iteration
            continue;
        }     

        //Things written from this point on act as "else"

    }

Seventeen

But what if I'm not in a loop or a function? What if I'm in a WordPress template partial? – Brim

then I cant think of a way of doing it that is cleaner than using "else" – Seventeen

Answer 4

Remove the else block by ending the 404 producing branch:

if ($route === null) { 
  // show 404 page
  $this->showErrorPage(404);
  return;
}

// a route was found
$route->dispatch();

Mindi

Answer 5

This answer is coming late, but another method you can get around that is by using else if. Because sometimes you cannot just return if some logic should follow.

Having your example

if ($route !== null) { // a route was found
    $route->dispatch();
}
else if ($route === null) {
    $this->showErrorPage(404);
}

$route->doSomething();

Lakin