PHP: 2000 words on elseif (and PHPMD)

G'day:
This is another one that just lives up to the definitive notion of a blog: a web log; a log of what I'm doing. But, like... web-based. A blog. What I basically mean is that I'm not sure I'm showing any insight here (look: I basically know I'm not), but it's what I looked at today, so it's what I'm writing about today.

We're trying to lift our game when it comes to our code "Cleanliness" (in the RC Martin "Clean Code" sense), and we've started more rigorously using some code review tools recently, such as PHPCS and PHPMD. They're both pretty good, and I recommend them to anyone using PHP. Almost everyone probably already is: I'm always late to these games.

One PHPMD rule that tends to irk me is the one that declares "[one's code] uses an else expression. Else is never necessary and you can simplify the code to work without else". Now: generally this is true. One can usually refactor one's code to stick it in a helper method and early return in the if logic, then just the rest of the code after the if block is what happens in the else case. This is good because it helps one's code be "closer to the left-edge"; ie: it's not indented because it's not in a logic block any more. The more one needs to indent one's code: the more it probably smells. It's got to the stage with our code now that a double nesting of logic-control statements almost gets the same reaction from code reviewers as when they see a comment in our code; "hey, gather round everyone: Adam's made a comment. Let's look at it!" (Adam shrinks into the carpet and starts getting his excuses ready. Later he refactors the code to be clearer, and the comment goes; the code reviewers are happy). The problem for me is that I'm not clever enough sometimes, and it takes me a coupla takes before I see how to get rid of a wayward else. At least now I usually pick it up and work it out during my "self code review" phase, and sort it out. And PHPMD helps with this.

Today I was reviewing a colleague's code, and noted they had not an else but an elseif. And I triumphantly(*) pinged them with a comment reminding them to run PHPMD before submitting their pull request.

(*) because I'm petty.

It turns out that they had run PHPMD, and it hadn't pulled 'em up. Initially (again: cos I'm petty) I didn't believe them and knocked together my own SSCCE and looked at it.

This demonstrates the problem:

function takeAChance(){
    $firstChance = (bool) rand(0,1);

    if ($firstChance) {
        echo "firstChance was true";
    } else {
        echo "firstChance was false";
    }
}

takeAChance();

And now running PHPMD over it:

C:\src\php\php.local\src\phpmd>phpmd else_block.php text phpmd.xml
C:\src\php\php.local\src\phpmd\else_block.php:8 The method takeAChance uses an else expression.
Else is never necessary and you can simplify the code to work without else.

C:\src\php\php.local\src\phpmd>

OK, fair enough. Oh btw, the solution to this is to early return:

function takeAChance(){
    $firstChance = (bool) rand(0,1);

    if ($firstChance) {
        echo "firstChance was true";
        return;
    }
    echo "firstChance was false";
}

(note: yeah yeah, in the real world I'd not be echoing from a function. I get that. Not the point here)

OK, now here's an SSCCE of the sort of thing we had in this pull request:

function takeAChance(){
    $firstChance = (bool) rand(0,1);
    $secondChance = (bool) rand(0,1);

    if ($firstChance) {
        echo "firstChance was true; don't care what secondChance was";
    }elseif($secondChance){
        echo "well at least secondChance was true";
    }
}

And running PHPMD:

C:\src\php\php.local\src\phpmd>phpmd elseif_block.php text phpmd.xml

C:\src\php\php.local\src\phpmd>

No problems. Grrr. This does not make sense to me. If we add an actual else in there:

function takeAChance(){
    $firstChance = (bool) rand(0,1);
    $secondChance = (bool) rand(0,1);

    if ($firstChance) {
        echo "firstChance was true; don't care what secondChance was";
    }elseif($secondChance){
        echo "well at least secondChance was true";
    }else{
        echo "both were false";
    }
}

And PHPMD says:

C:\src\php\php.local\src\phpmd>phpmd if_elseif_else_block.php text phpmd.xml
C:\src\php\php.local\src\phpmd\if_elseif_else_block.php:11
The method takeAChance uses an else expression.
Else is never necessary and you can simplify the code to work without else.

C:\src\php\php.local\src\phpmd>

And, yeah, it's true... but the elseif seems to me to be as much an issue here as an else. We can short-circuit both easily enough with early returns:

function takeAChance(){
    $firstChance = (bool) rand(0,1);
    $secondChance = (bool) rand(0,1);

    if ($firstChance) {
        echo "firstChance was true; don't care what secondChance was";
        return;
    }
    if($secondChance){
        echo "well at least secondChance was true";
        return;
    }
    echo "both were false";
}

Equally, to me a PHP elseif (note this is an actual command, it's not a concatenation of else if like one might expect), really is, functionally, a "single-statement else", just with the statement itself being an if statement. What I mean is remember that the if / elseif / else statement doesn't need to use blocks (ie: a bunch of statements within braces), it could be a single statement:

function takeAChance() {
    $firstChance = (bool) rand(0,1);

    if ($firstChance)
        echo "firstChance was true";
    else
        echo "firstChance was false";

}

(what an abomination: almost looks like Python. Bleah).

Anyway... so yeah, one doesn't need to use braces if there's only one statement to execute based on the result of the condition. Don't do this. But it's possible.

What I'm getting at is this:

if ($firstChance) {
    echo "firstChance was true; don't care what secondChance was";
}elseif($secondChance){
    echo "well at least secondChance was true";
}

Is basically like this:

if ($firstChance) {
    echo "firstChance was true; don't care what secondChance was";
}else{
    if($secondChance){
        echo "well at least secondChance was true";
    }
}

Just without the braces on the else:

if ($firstChance) {
    echo "firstChance was true; don't care what secondChance was";
}else
    if ($secondChance){
        echo "well at least secondChance was true";
    }

Compare this again to the elseif version:

if ($firstChance) {
    echo "firstChance was true; don't care what secondChance was";
}elseif ($secondChance){
    echo "well at least secondChance was true";
}

The only difference there is the whitespace between the else and the if. And logically there is no difference, as far as I can tell.

I tested all variations I could think of of elseifs, elses, braces, no braces, and the like (see the samples on GitHub), and ran PHPMD over them:

C:\src\php\php.local\src\phpmd\samples>phpmd . text phpmd.xml
C:\src\php\php.local\src\phpmd\samples\else_block.php:8 The method takeAChance uses an else expression. Else is never necessary and you can simplify the code to work without else.
C:\src\php\php.local\src\phpmd\samples\else_if_block.php:9      The method takeAChance uses an else expression. Else is never necessary and you can simplify the code to work without else.
C:\src\php\php.local\src\phpmd\samples\if_elseif_else_block.php:11      The method takeAChance uses an else expression. Else is never necessary and you can simplify the code to work without else.

C:\src\php\php.local\src\phpmd\samples>

Hmmmm... it's not a case of else clauses being a problem it seems. It's a problem with else blocks being a problem.

It's complaining about this:

if ($firstChance) {
    echo "firstChance was true";
} else {
    echo "firstChance was false";
}

But not about this:

if ($firstChance) {
    echo "firstChance was true";
} else 
    echo "firstChance was false";
 

The only difference being the lack of braces on the second one.

Now I guess this makes an element of sense. The "untidiness" of large logic blocks is that they are more of a pain to navigate across when maintaining the code. With an if/else statement one needs to pay attention to where the if is, where the else is, which code is in which clause, and where the whole thing ends. This is not a problem if one keeps the code clean so each block has only 1-2 statements in it, but we've all seen if/elses that go on for miles and miles worth of scrolling down (and, it's worth pointing out: that is shit code if it's written like that. Don't do it). It's exacerbated by further nested logic within that. Fair enough.

But with a non-block else case - which intrinsically can only be one statement - the problem of unclean block sizes can simply not occur. Also fair enough.

But in that case, the warning should not be able else, intrinsically; it should be about the size of the blocks. And the size rule should probably apply to the if block too! If a single-statement else is OK, then a single-statement else block should be OK too.

I think the solution to this is that any else clause should be flagged: be it a single statement, or be it a block. So intrinsically an elseif should be flagged too. I'll raise this with the PHPMD bods.

Whilst on this: they should probably swing the other way and actually flag any instances of single-statement logic clauses: ifs and loops should always have braces. That's a widely accepted truism in curly-brace-language programming. I realise PHPCS does pick up on these (I only realise it just now cos I just tested it), but if PHPMD is busying itself with this sort of thing too; it should do a thorough job. In my view brace-less control logic is mess.

---

Lastly on this, I'm slightly irked by this, from the PHPMD Clean Code Rules page:

ElseExpression

[...]

An if expression with an else branch is never necessary.

It is terribly pedantic of me, but… it's not an expression. PHP flow control statements are…statements, not expressions. An expression is a specific thing: it is some code that represents a value. In PHP if/else statement does not represent a value. One cannot do this:

$message = if ($firstChance) {
    "firstChance was true";
} else {
    "firstChance was false";
}

If you cannot have some code on the right-hand side of an assignment (or anywhere else a value might be expected), then it's not an expression. A counterpoint to this, one can do this in Ruby:

message = if firstChance
    "firstChance was true";
else
    "firstChance was false";
end

puts message

This is because "everything is an object" in Ruby, I guess: even statements. In this case it seems the value of the if/else statement is that of the last statement in the relevant clause that gets executed by the logic. I'm buggered if I know whether Ruby best practices would encourage that (and by that I mean: I really actually don't know!), but I dunno that I'd be doing that sort of thing. Still: it demonstrates an if statement that is also an expression. Which a PHP one is not.

This sort of thing wouldn't bug me if it was in a casual conversation, but in an official implementation, I think it's a bit… ah… well let's saynit could just be done properly. But then again I guess this is PHP. And "doing it right" doesn't seem to be something PHP concerns itself with too much. It seems to be more "just get it done".

All this aside, I think PHPMD is a bloody handy tool, and whether or not I agree with the minutiae of some of its analysis or its turn of phrase is minor. It's helped my code a lot in the last coupla weeks. And I think our code in general is better of for using tools like this.

So… yeah. That was what I was doing yesterday (it was Fri when I started, it's now Sat evening and I'm in the pub). not scintillating stuff, but the examination of what was going on and the general experimentation was useful (to me, anyhow), I think. And it's enabled me to write a 2000-word blog article which is basically about elseif expressionsclauses.

Righto.

--
Adam