3
\$\begingroup\$

Can someone take a look over my bootstrap file and see if you believe it's doing too much or the logic could be better.

<?php

    /*
    * ------------------------------------
    * Autoloader for classes/files
    * ------------------------------------
    */

    function __autoload($class) {
        if(file_exists($file = str_replace("\\","/",str_replace("baremvc\\","",$class)).".php")){
            require_once $file;
        }else{
            throw new Exception($file);
        }
    }

    /*
    * ------------------------------------
    * Get commonly used namespaces
    * ------------------------------------
    */

    use \baremvc\core\library\exception as ex;
    use \baremvc\core\configuration as config;

    /*
    * ------------------------------------
    * Enable session if set in the configuration file
    * ------------------------------------
    */

    !config::enable_sessions ?: session_start();

    /*
    * ------------------------------------
    * Enable debug information if set in the configuration file
    * ------------------------------------
    */

    error_reporting(config::debug_level);
    ini_set('display_errors', intval(config::debug_enabled));

    /*
    * ------------------------------------
    * Try and load the specified class/method
    * ------------------------------------
    */

    $router = new \baremvc\core\library\router;
    $url_object = $router->parse_request_url(@$_SERVER['REDIRECT_URL']);
    try{
        $namespace = "\\baremvc\\controllers\\{$url_object->Controller}";
        $class = new $namespace;
        if(method_exists($class,$url_object->Method)){
            if(!empty($url_object->Get)){
                call_user_func_array(array($class, $url_object->Method), $url_object->Get);
            }else{
                call_user_func(array($class, $url_object->Method));
            }
        }else{
            throw new Exception($url_object->Method);
        }
    }catch(Exception $e){
        echo ex::show_404($e->getMessage());
    }

parse_request_url takes the requested url (e.g. domain.com/this/is/my/request) and checks to see if a controller exists with that name (can be in directories or file).

\$\endgroup\$
  • 1
    \$\begingroup\$ I'd recommend using DIRECTORY_SEPARATOR instead of just assuming that file paths use / as the separator (I'd also recommending aliasing it to something shorter ;) ). Also, I'd replace file_exists with is_file as file_exists would return true if the item exists but isn't a file. Finally, spl_autoload_register is a better choice than __autoload as it will still work even if another autoloader has been defined. I treat __autoload as if it's deprecated. \$\endgroup\$ – GordonM Jan 16 '12 at 20:02
  • \$\begingroup\$ Actually there is no need for DIRECTORY_SEPARATOR as long as one uses the forward slash /. Windows doesn't mind and *nix system already use the forward slash. @source alanhogan.com/tips/php/directory-separator-not-necessary \$\endgroup\$ – user555 Nov 9 '12 at 1:16
5
\$\begingroup\$

Generally this is very good code, but I try to be thorough when reviewing so I've listed every minor point that I could think of.

It looks like you write OO code in other places, but you chose to make your bootstrap procedural. Believe it or not there are benefits to an OO bootsrap! You can define methods for different parts of the system that you want to bring up. There will always be some procedural code required to call the bootstrap, but it could look more like this:

$bootstrap = new Bootstrap();
$bootstrap->initializeAutoload();
$bootstrap->initializeErrorReporting();
$bootstrap->initializeRouter();

As GordonM said, use spl_autoload_register.

You are tightly bound to the config class by your static calls to it. This makes it hard to test your bootstrap. It is understandable in the bootstrap as you have only just brought the autoload up. Keep reading and at the end I'll tell you what I recommend on that.

Personally I don't like the error control operator @. I find it hard to understand what will happen when your server redirect url is not defined. This is very minor, but I would use:

if (isset($_SERVER['REDIRECT_URL']))
{
    $url_object = $router->parse_request_url($_SERVER['REDIRECT_URL']);
}
else
{
    $url_object = $router->parse_request_url('');

}

$namespace and $class are misleading to me, I would rewrite those:

$class_name = '\baremvc\controllers\\' . $url_object->Controller;
$controller = new $class_name;

ex::show_404 couples your code tightly to the ex class. This makes it harder for you to test this code as now you require a stub for ex as well as config. You also call new to create a variable controller class, that would make it hard to test.

Other good things that you can do in the bootstrap are: set_error_handler, set_exception_handler, register_shutdown_function.

Now about some of the coupling in this code (to config, ex and the variable controller class). This is an issue if you want to test this code or have it so that it is reusable (You are going to need config and ex if you want to reuse it as it is). You may be happy enough to leave it as it is. That would be a valid choice and one many people would take. Many people should stop reading now (I won't be offended if you also ignore the following).

Now, if you want to get around it a little. All static calls and calls to new bind the calling code to the class that they are calling. Look at the following and answer for yourself how tightly they bind the code to the class:

config::enable_sessions
$object->enable_sessions
$configObject = new Config();
$configObject = $container->getNew('Config');

I believe the tightness of coupling is - High (need config class with enable_sessions), Low (need any object with enable_sessions), High (need config class), Low (need an object with getNew).

My recommendation would be that if you are going to bind your Bootstrap to anything it should be a container class, because from that you can get any other object that you need. This means you will only have to stub one class (the container) when you test, which should be simple.

\$\endgroup\$
  • \$\begingroup\$ Thank you VERY much for all of your suggestions - very clear and some good ideas that I will be implementing. \$\endgroup\$ – user10030 Jan 18 '12 at 14:46
  • \$\begingroup\$ No problems, welcome to code review. \$\endgroup\$ – Paul Jan 19 '12 at 3:29
  • \$\begingroup\$ Somehow I always thought that bootstrap files were supposed to be procedural, but your suggestion of making it OO looks very good and logical when it comes to OOP. I made an attempt of my own, if you're interested to review it. \$\endgroup\$ – Kid Diamond Jun 6 '14 at 13:16

Your Answer

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