PHP Mess Detector giving false positives

Question

I'm working with an open source project and thought it would be a good idea to implement automated code revisions with phpmd.

It showed me many coding mistakes that im fixing already. But one of them made me curious.

Consider the following method:

/**
 * 
 * @param string $pluginName
 */
public static function loadPlugin($pluginName){
    $path = self::getPath()."plugins/$pluginName/";
    $bootPath = $path.'boot.php';
    if(\is_dir($path)){

        //Autoload classes
        self::$classloader->add("", $path);

        //If theres a "boot.php", run it
        if(is_file($bootPath)){
            require $bootPath;
        }

    }else{
        throw new \Exception("Plugin not found: $pluginName");
    }
}

Here, phpmd says that Else is never necessary

...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. ...

is_dir will return false whenever the given path is a file or simply does not exist, so, in my opinion, this test is not valid at all.

Is there a way to fix it or maybe simply ignore cases like this one?

Just a tip. There is Sniff alternative in CodeSniffer: https://github.com/object-calisthenics/phpcs-calisthenics-rules#1-only-x-level-of-indentation-per-method — Tomas Votruba, May 03 '17 at 09:55

score 2 · Answer 1 · edited May 23 '17 at 12:18

I don't use phpmd, but it's clear that your if statement is a guard clause. Guard clauses don't need else branches, you can safely refactor your code like this:

/**
 * @param string $pluginName
 * @throws \Exception if plugin cannot be found
 */
public static function loadPlugin($pluginName)
{
    $path = self::getPath() . "plugins/$pluginName/";
    if (!\is_dir($path)) {
        throw new \Exception("Plugin not found: $pluginName");
    }

    // Autoload classes
    self::$classloader->add("", $path);

    // If there is a "boot.php", run it
    $bootPath = $path . 'boot.php';
    if (is_file($bootPath)) {
        require $bootPath;
    }
}

Further reading:

score 1 · Accepted Answer · edited Feb 25 '16 at 02:24

1

An alternative to the structure is something like this:

public static function loadPlugin( $pluginName ) {
    $path = self::getPath() . "plugins/$pluginName/";
    $bootPath = $path . 'boot.php';
    if( \is_dir( $path ) ) {
        // Autoload classes
        self::$classloader->add( "", $path );
        // If theres a "boot.php", run it
        if ( is_file( $bootPath ) ) {
            require $bootPath;
        }
        // A return here gets us out of the function, removing the need for an "else" statement
        return;
    }

    throw new \Exception( "Plugin not found: $pluginName" );
}

While I'm not sure it's the solution, it is a technique to avoid the else condition. Else conditions can add complexity when trying to read the code, and allowing the function to "flow" without else conditions can make them more readable.

edited Feb 25 '16 at 02:24

CarlosCarucce

3,420
1
28
51

answered Feb 25 '16 at 01:58

random_user_name

25,694
7
76
115

Its a good solution, the warning does not shows up. But I had to move the `return` outside that if, because that file is optional in the plugin structure. – CarlosCarucce Feb 25 '16 at 02:20
1

In any case, I dont think just removing all the _else_ clauses is a good idea, since its a basic decicion statement present in all programming languages. Do you know if there's a way of disabling this test in PHPMd? – CarlosCarucce Feb 25 '16 at 02:24
1

I personally don't use PHPMd. I have grown to absolutely **love** PHPStorm (it's an IDE). It's SO good, and it provides code formattting tools, hints, suggestions, etc. My code is way better since using it. – random_user_name Feb 25 '16 at 02:26
Thanks for the suggestions! – CarlosCarucce Feb 25 '16 at 02:38

PHP Mess Detector giving false positives

2 Answers2