patternphpMinor
Bootstrap file - safe and correct
Viewed 0 times
filebootstrapcorrectsafeand
Problem
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.
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).Solution
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:
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:
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
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.
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.
Code Snippets
$bootstrap = new Bootstrap();
$bootstrap->initializeAutoload();
$bootstrap->initializeErrorReporting();
$bootstrap->initializeRouter();if (isset($_SERVER['REDIRECT_URL']))
{
$url_object = $router->parse_request_url($_SERVER['REDIRECT_URL']);
}
else
{
$url_object = $router->parse_request_url('');
}$class_name = '\baremvc\controllers\\' . $url_object->Controller;
$controller = new $class_name;config::enable_sessions
$object->enable_sessions
$configObject = new Config();
$configObject = $container->getNew('Config');Context
StackExchange Code Review Q#7867, answer score: 5
Revisions (0)
No revisions yet.