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?

score 47 · Answer 1 · edited Dec 11 '16 at 16:04

47

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)
 */

edited Dec 11 '16 at 16:04

Christian

27,509
17
111
155

answered Dec 09 '15 at 22:09

Abin

1,699
1
14
17

2

Is there any paper or any kind of reference material one can take a look to try and understand this rule of no else? – AntonioCS May 06 '18 at 17:41
1

@AntonioCS you can find the rule details here: https://phpmd.org/rules/cleancode.html#elseexpression. PSR-2 recommendation on the same can be found here. https://github.com/php-fig-rectified/fig-rectified-standards/blob/master/PSR-2-R-coding-style-guide-additions.md – Abin Jun 16 '18 at 16:53
2

The rules actually say to use Else statements. https://github.com/php-fig-rectified/fig-rectified-standards/blob/master/PSR-2-R-coding-style-guide-additions.md#ternary-operator – AntonioCS Jun 17 '18 at 21:05
2

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: https://github.com/php-fig-rectified/fig-rectified-standards/blob/master/PSR-2-R-coding-style-guide-additions.md#writing-better-code * Try to return early in methods/functions to avoid unnecessary depths – Abin Jun 19 '18 at 05:43
5

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 – AntonioCS Jun 20 '18 at 07:18
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. – Bezerra Jan 06 '20 at 13:18

Joaquin Brandan · Answer 2 · 2018-12-28T18:39:46.477

6

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"

    }

edited Dec 28 '18 at 18:39

answered Dec 28 '18 at 18:30

Joaquin Brandan

2,663
3
20
21

But what if I'm not in a loop or a function? What if I'm in a WordPress template partial? – robrecord Jul 09 '19 at 14:07
1

then I cant think of a way of doing it that is cleaner than using "else" – Joaquin Brandan Jul 10 '19 at 17:38

score 2 · Accepted Answer · answered Sep 20 '15 at 07:58

2

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) ;

answered Sep 20 '15 at 07:58

gyaani_guy

3,191
8
43
51

So is there something better than phpMD? – Pascal Sep 20 '15 at 08:04
4

learning from http://www.phptherightway.com/ is often suggested by many programmers – gyaani_guy Sep 20 '15 at 08:08
1

How is that cleaner? maybe at a return statement or something but other than these are exactly the opposite of clean code – nights Dec 11 '19 at 04:40

score 1 · Answer 4 · answered Dec 11 '19 at 04:43

1

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();

answered Dec 11 '19 at 04:43

nights

411
3
9

score 0 · Answer 5 · answered Jan 28 '22 at 11:59

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();

Why does clean code forbid else expression

5 Answers5