Pluggin' in Plugins

Question

I'm currently working on an in-house project and a part of that project is this pretty standard plugin loader below.

There must be a way to avoid so many ugly looking else statements.

I'm kinda hitting a brick wall with getting better at "writing less to achieve the same thing".

How I can improve the code as I do think it's pretty clean, but I got this aching feeling that I've been missing a lot of "shorthanding" my code?

/**
 * Load plugins specified by the array $load, containing the leading filenames in the /src/plugins/ folder
 * All plugins must be proceeded with *.plugin.php where the asterisk is the name of the respective plugin
 *
 * @since 0.1
 * @param array $load e.g: array("Users", "Products", "Pages")
 * @return void
 */
public function __construct($load) {

    $this->plugin_dir = $_SERVER['DOCUMENT_ROOT'] . "/PlugCMS/src/plugins";

    require_once($this->plugin_dir . "/Users.default.php"); // Included by default, and is required to gain access to the dashboard
    require_once($this->plugin_dir . "/Pages.default.php"); // You can remove this but you will need to remove the pageLoader.php mod_rewrite rule from .htaccess to get control of the front-end back.

    if (is_dir($this->plugin_dir)) {            
        if (is_array($load)) {

            $count = count($load);

            for ($i=0; $i < $count; $i++) {

                $filename = $this->plugin_dir . "/" . $load[$i] . ".plugin.php";

                if (file_exists($filename)) {

                    if (@require_once($filename)) {
                        $this->load_output['active'][] = $load[$i];
                    }
                    else
                    {
                        $this->load_output['failed'][] = $load[$i];
                    }

                }
                else
                {
                    $this->load_output['failed'][] = $load[$i];
                }
            }

        }
        else
        {
            $this->load_output['error'][] = "Error: $load must be an array";
        }   
    }
    else
    {
        $this->load_output['error'][] = "PlugCMS has detected the default plugin directory is missing ({$this->plugin_dir}). Please check the installation and try again..";
    }

}

Answer 1

I think surely there must be a way to avoid so many ugly looking else statements.

Return early, for example:

if (is_dir($this->plugin_dir)) {
    $this->load_output['error'][] = "PlugCMS has detected the default plugin directory is missing ({$this->plugin_dir}). Please check the installation and try again..";
    return;
}

You can do the same for is_array($load), and already you got rid of two levels of nesting.

You can also combine file_exists($filename) and @require_once($filename) to file_exists($filename) && @require_once($filename) to get rid of another level of nesting.

Answer 2

I'm aware of return early but the opportunity is unfortunately quite rare

Two levels removed as @tim's suggestions, it's quite significant. With his other suggestions applied too, the whole thing becomes quite a bit shorter:

if (!is_dir($this->plugin_dir)) {            
    $this->load_output['error'][] = "PlugCMS has detected the default plugin directory is missing ({$this->plugin_dir}). Please check the installation and try again..";
    return;
}
if (!is_array($load)) {
    $this->load_output['error'][] = "Error: $load must be an array";
    return;
}   

$count = count($load);

for ($i = 0; $i < $count; $i++) {
    $filename = $this->plugin_dir . "/" . $load[$i] . ".plugin.php";

    if (file_exists($filename) && @require_once($filename)) {
        $this->load_output['active'][] = $load[$i];
    } else {
        $this->load_output['failed'][] = $load[$i];
    }
}

In addition, I think you can replace this:

if (file_exists($filename) && @require_once($filename)) {

With this:

if (@include_once($filename)) {

Because if the file doesn't exist, the include_once will fail anyway.

Originally I suggested @require_once there, but that would be bad, as @tim has pointed out:

As require_once will halt the script on failure, only using if (@require_once($filename)) will not work (the script will stop completely, and the else statement will not be executed). So either the file_exists should be kept, or include_once should be used (this probably makes more sense, but might be a bit harder to read).

Answer 3

I know this is old, but I feel it important to note that include, require and their respective *_once statements are language constructs, not functions. As such, you should stylistically avoid using brackets:

require_once $this->plugin_dir . "/Users.default.php";

It will work with them, but it's considered bad practice to use them.