20
\$\begingroup\$

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..";
    }

}
\$\endgroup\$
  • 2
    \$\begingroup\$ Argh... seeing code that contains @require_once really makes my blood boil. Please don't use the @ operator of death: if you see a warning/notice/error, FIX IT, don't silence it. \$\endgroup\$ – Elias Van Ootegem Oct 23 '14 at 19:20
  • \$\begingroup\$ @Elias The default plugins stop on error, the user-defined ones are silences as it's not worth it causing the site to halt just because someone messed up a plugin, they can debug outside of the plugin loader. \$\endgroup\$ – zanderwar Oct 23 '14 at 23:15
  • \$\begingroup\$ I have changed it to @include_once too as suggested by @tim and @janos \$\endgroup\$ – zanderwar Oct 23 '14 at 23:25
  • \$\begingroup\$ That's not the point. The point is your use of @. If an include produces an error/warning/notice, then you should work out why it does so, and fix the root cause. Supressing the error doesn't fix the problem. Think of it as cleaning your house: you can wipe the dirt under the carpet so you don't see it, but it's still there; or you can get a vacuum cleaner and remove it entirely. The latter is the cleaner option. \$\endgroup\$ – Elias Van Ootegem Oct 24 '14 at 6:32
16
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your reply, I'm aware of return early but the opportunity is unfortunately quite rare, touché on combining the file_exists() and require_once(). Would anyone have a reference URL with something similar to add to what Tim said; but not limited to? I've come up NULL on anything decent throughout my searches. \$\endgroup\$ – zanderwar Oct 23 '14 at 13:23
  • 1
    \$\begingroup\$ you have an extra Curly Brace in there?? \$\endgroup\$ – Malachi Oct 23 '14 at 14:49
  • 3
    \$\begingroup\$ @Zanderwar try searching for reduce deeply nested if statements (no reason to include a language, the principles are pretty much universal), there are quite a few results for it. \$\endgroup\$ – tim Oct 23 '14 at 17:02
15
\$\begingroup\$

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

\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.