else is never necessary and you can simplify the code to work without else

Question

I got a message of PHPMD telling me:

else is never necessary and you can simplify the code to work without else on this portion of code:

if ($settings == null) {
        $settings = new self($arrSettings);
} else {
        $settings->fill($arrSettings);
}
$settings->save();

return $settings;

My question is : How should I avoid else(). The only way I see is duplicating $setting->save() and return.

Any idea?

Answer 1

TL,DR

• PHPMD has a point
• Intentions are good, but PHPMD's advice that else "is never necessary" is wrong
• You can always remove else -- but many times this will result in messier code (which is ironic, since you will be following the advice of a "mess detector").
• So you should always ask yourself: should I avoid this else?
• Even PHPMD itself uses else so it looks that sometimes it is necessary

Long answer

else is a pretty standard language construct, used in many language and software packages (including PHPMD itself). To me, saying that else is never necessary is the same as saying that do...while is never necessary because you can combine while (true) and break, which it is true, but pointless. So I'd take that advice with a pinch of salt, and reflect a little bit before changing any software based solely on a static analyzer recommendation.

All that said, what I think PHPMD developers meant was that, in many cases, you can and should remove else from your code to make it more readable.

Simplest cases are:

    if ($condition) {
        $a = 1;
    } else {
        $a = 2;
    }

Which can be simplified to:

    $a = ($condition ? 1 : 2);

Now look at this expression:

    // Calculate using different formulas, depending on $condition.
    if ($condition) {
        // Calculate using secret formula.
        $a = power($r * M_PI, 2) / sqrt(exp($k));
    } else {
        // Calculate using standard formula.
        $a = power(($r / $k) * M_PI, 2) / sqrt(1 / $k);
    }

This can be changed to:

    $a = ($condition ? power($r * M_PI, 2) / sqrt(exp($k)) : power(($r / $k) * M_PI, 2) / sqrt(1 / $k));

Granted, the second forms are more succint or, I should say, small. But from a code clarity and maintainability perspective, I guess the original code with an else is far more clear, not to mention that explaining the "improved" versions using code comments is far more difficult, isn't?

IMHO, it is. And I always use else in cases like that.

Another simple example:

    // Check animal first.
    if ($animal === 'dog') {
        if ($breed === 'pinscher') {
            $weight = 'light';
        } else {
            $weight = 'heavy';
        }
    } else {
        // We only deal with dogs.
        $weight = "we cannot say anything about $animal";
    }

Version without else:

    $weight = ($animal === 'dog' ? ($breed === 'pinscher' ? 'light' : 'heavy') : "we cannot say anything about $animal");

Notice that in this case, the version without else is in direct violation o PSR-2, which forbids nested ternary operators.

Oftentimes one keeps simple constructs that otherwise could be replaced with a ternary operator, just because one wants to avoid long lines in your code, which are detrimental to code readability:

    if (sqrt($weight) > 30 && $animal === 'elephant' && $location == 'zoo') {
        $visitors_max = sqrt($guards) / ($ticker_price * M_2_SQRTPI)
    } else {
        $visitors_max = $visitors_max_last_year * exp($ticket_price) * 1.1;
    }

That becomes:

    $visitors_max = (sqrt($weight) > 30 && $animal === 'elephant' && $location == 'zoo') ? sqrt($guards) / ($ticker_price * M_2_SQRTPI) : $visitors_max_last_year * exp($ticket_price) * 1.1);

Moving on, here is another well known pattern that I suppose PHPMD wants you to addresses:

    function myfunc($arg)
    {
        if ($arg === 'foo') {
            $res = 'foo found';
        } else {
            $len = strlen($arg);
        if ($len > 10) {
            $res = 'arg is too big';
        } else {
            $bar = dosomething($res);
            $res = "$arg results in $bar";
        }
        return $res;
    }

This function uses an advice once taught on programming classes that functions should have a single exit point, as that (arguably) make it easier to understand program flow and find bugs.

IMHO (and PHPMD's), we can remove else and improve code clarity/maintainability in code like this without losing anything:

    function myfunc($arg)
    {
        if ($arg === 'foo') {
            return 'foo found';
        }

        $len = strlen($arg);
        if ($len > 10) {
        return 'arg is too big';
        }

        $bar = dosomething($res);
        return "$arg results in $bar";
    }

But this may not always be desirable:

    function mycalc($n)
    {
        if ($n === 0) {
           $multiplier = 0.5;
        } elseif ($n === 1) {
           $multiplier = M_LN2;
        } else {
           $multiplier = power(sqrt($n * M_PI), 2);
        }

        return $multiplier * (M_2_PI * power($n * M_PI, 2));
    }

An "improved" version should be something like:

    function mycalc($n)
    {
        if ($n === 0) {
           return 0.5 * (M_2_PI * power($n * M_PI, 2));
        }

        if ($n === 1) {
           return M_LN2 * (M_2_PI * power($n * M_PI, 2));
        }

        return power(sqrt($n * M_PI), 2) * (M_2_PI * power($n * M_PI, 2));
    }

I am not sure about you, but the first version follows my train of thought about the calculation much more closely, thus is much more easy to understand and maintain than the second one, even though it uses the "forbidden" else.

(One may argue that we could use the second form, plus an auxiliary variable to hold the common calculation. Fair enough, but one can always argue back that adding an unnecessary variable makes code less clear and costly to maintain.)

So, to answer your question how should I avoid else?, I will ask another one: why should you?

@tereško answer makes some sense, and indeed makes the code more concise. However, I personally think that your first version is perfectly fine, more explicit in its intentions, thus much better from an understandability and maintainability POV:

    if (I do not have $object)
        create a new $object with my settings
    else
        call the "fill" method of $object with my settings
    endif
    do stuff with $object

Versus:

    if (I do not have $object)
        create a new $object
    endif

    call the "fill" method of $object with my settings
    do stuff with $object

Also notice that there is a subtle change in programming logic in the version without the else above: you (and all future developers) must assume that call the "fill" method of $object with my settings and create a new $object with my seetings always end up with an object with the same internal state. This assumption is not needed in the original version.

In other words, the refactored code will work as long as the fill() method and the object's constructor do the same thing to the object's internal state, which may or may not be true -- now or ever.

To illustrate this point, suppose that the object is defined as this:

    class MyClass
    {
        protected $fillCount = 0;

        protected $settings;

        public function __construct(array $settings)
        {
            $this->settings = $settings;
        }

        public function fill(array $settings)
        {
            $this->fillCount++;
            $this->settings = $settings;
        }
    }

In this case, your original version and the one without the else will end up with objects with different internal states, and finding a bug is going to be a bit more difficult, because it will be hidden behind assumptions and implicit constructions.

Now, let's take a look at one PHPMD's own else:

    // File: src/bin/phpmd

    if (file_exists(__DIR__ . '/../../../../autoload.php')) {
        // phpmd is part of a composer installation
        require_once __DIR__ . '/../../../../autoload.php';
    } else {
        require_once __DIR__ . '/../../vendor/autoload.php';

        // PEAR installation workaround
        if (strpos('@package_version@', '@package_version') === 0) {
            set_include_path(
                dirname(__FILE__) . '/../main/php' .
                PATH_SEPARATOR .
                dirname(__FILE__) . '/../../vendor/pdepend/pdepend/src/main/php' .
                PATH_SEPARATOR .
                '.'
            );
        }
    }

    // (...100+ lines of code follows...)

The question is: should we avoid this else?

Answer 2

Probably because it can be re-written as

if ($settings === null) {
    $settings = new self; // or new self([]);
}
$settings->fill($arrSettings);
$settings->save();

return $settings;

But, TBH, the entire thing looks like one big violation of SRP, because class instances should not be able to crate new instances of themselves. That just don't make any sense .. but then again, I am not an "artisan".