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?

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

Answer 2

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:

• Use Guard Clauses for all the special cases
• "Return early" vs "Minimize returns" balance