Add unnecessary else rule?

[dingo-d]

This code for instance

if ( $some_variable === 'some_string' ) {
    $var = 'one thing';
} else {
    $var = 'another thing';
}

Can be shortened to

$var = 'another thing';

if ( $some_variable === 'some_string' ) {
    $var = 'one thing';
}

But I haven't seen a rule about this.

Should this be a rule? I couldn't find anything about it, but it seems like it's a good practice.

Thoughts?

[aristath]

I agree 100% with this one. else is never necessary and should be avoided

[GaryJones]

else is never necessary and should be avoided

While it's can usually be avoided, saying it's never necessary is not true:

if ( $foo ) {
    $var = 'simple string';
} else {
    $var = my_expensive_db_or_http_query();
}

Changing that to:

$var = my_expensive_db_or_http_query();

if ( $foo ) {
    $var = 'simple string';
}

makes the expensive function be always called, potentially unnecessarily. Sure, the code might be easier for you to read, but it shouldn't be at the expense of poorer performance for all the users.

There is no rule in the Handbook for this, and nor would I expect to be at this stage. It could potentially go as a warning in WordPress-Extra, but even then, as shown, it's not really absolute or "warning-worthy", and adding in annotated exclusions to avoid the warnings makes the code harder to read than leaving some else's in there.

Something like the awesome Php Inspections (EA Extended) plugin for PhpStorm has exactly this same check (amongst many others), and I suspect Scrutiziner and similar tools would also pick this up - as much as I love the result of this check, I don't think WPCS is the right place for it.

[aristath]

In the above scenario I can think of 2 alternatives that both seem better to me... Instead of this:

if ( $foo ) {
    $var = 'simple string';
} else {
    $var = my_expensive_db_or_http_query();
}

it would be preferable to do this:

$var = 'simple string';
if ( ! $foo ) {
    $var = my_expensive_db_or_http_query();
}

Or even

$var = ( $foo ) ? 'simple string' : my_expensive_db_or_http_query();

else is still not necessary... EVER! As seen on https://phpmd.org/rules/cleancode.html

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. To achieve this you may need to split the code it several smaller methods. For very simple assignments you could also use the ternary operations.

[GaryJones]

it would be preferable to do this:

That's a fair comment - not sure why I hadn't thought of that!

[jrfnl]

I don't agree that else can always be avoided and I don't think this should be a hard rule, let alone a sniff.

I can think of several examples - even within the codebase of WPCS - where an if/else construct is the better choice for readability and clarity, to avoid code duplication, to avoid unnecessary extra function calls or to avoid having to abstract to a secondary function and needing to pass twenty variables to it for it to be able to work.

I do agree that it is often better not to use it and that people should consider carefully if code could benefit from not using else, but making it a hard rule + sniff, IMHO, goes too far for WPCS.

For those interested, you are free to use sniffs from other rulesets in your custom ruleset. You may be interested in this sniff library: https://github.com/object-calisthenics/phpcs-calisthenics-rules#2-do-not-use-else-keyword

Just some examples of why else should not be forbidden - keep in mind, return-ing early out of the function is not an option in these cases:

// Rewriting this would force a second unnecessary function call in half the situations
// or alternatively an extra interim variable which is not needed:
if ( ! empty( $select_index_end_cols ) ) {
    $max_index_width = max( $select_index_end_cols );
} else {
    $max_index_width = max( $index_end_cols );
}

// And what about this:
if ( $this->tokens[ $i ]['content'] === $this->phpcsFile->eolChar ) {
    $newlines++;
} else {
    $spaces += $this->tokens[ $i ]['length'];
}

// Or this:
if ( $nextNonWhitespace !== $opener ) {
    // Don't auto-fix: Something other than whitespace found between keyword and open parenthesis.
    $this->phpcsFile->addError( $error, $stackPtr, $error_code );
} else {
    $fix = $this->phpcsFile->addFixableError( $error, $stackPtr, $error_code );
    // Fixer code
}

[GaryJones]

Since it's not in the Handbook, and not desired for the WPCS ruleset (nothing to stop you using it in your own projects though - I happen to use the ObjectCalisthenics for instance), then I'm closing this one out.