Problem
I started off with a simple dispatch class, but it’s grown and I’m sure it could be simplified in the very least. In fact, I’m doing way too much in the __construct() method
It it being called as such:
$router = new router($_SERVER['REQUEST_URI'], $database);
$router->dispatch();
Yep, I’m pushing my database object into it.
Here’s the class:
class router
{
/**
* @var array Request URL
* @var array|string Controller Class Name
* @var array|string Method Name
* @var array|string Parameters
* @var array|bool|string Route Validity
* @var array|bool|string Primary Source
* @var ZendDbAdapterAdapter Database Adapter
*/
protected $url,
$controller,
$method,
$params,
$validRoute,
$primary,
$database;
/**
* Constructs the router class with routing paramters
* and database to inject into dispatched controller.
* Construct also sets the appropriate controller, method
* based in routing params, then checks if route is valid.
* @param $params
* @param $database
*/
public function __construct($params, ZendDbAdapterAdapter $database)
{
$this->database = $database;
$this->url = explode('/', trim($_SERVER['REQUEST_URI'], '/'));
$this->validRoute = true;
$requestParams = $_REQUEST;
unset($requestParams['url']);
$this->controller = !empty($this->url[0]) ? 'controller\' . $this->url[0] : 'controller\home';
if ($this->url[0] != 'ajax') {
$this->method = !empty($this->url[1]) ? $this->url[1] . "View" : 'indexView';
$this->params = !empty($this->url[2]) ? $_REQUEST : $_REQUEST;
$GLOBALS['url'] = array();
$GLOBALS['url'][] = !empty($this->url[0]) ? $this->url[0] : 'home';
$GLOBALS['url'][] = !empty($this->url[1]) ? $this->url[1] : '';
$GLOBALS['url'][] = !empty($this->url[2]) ? $this->url[2] : '';
} else {
$this->controller = 'catalina\ajax';
$this->method = 'handleRequest';
$this->params = array (
'url' => $this->url,
'controller' => !empty($this->url[1])
? 'controller\' . $this->url[1] : 'controller\home',
'method' => !empty($this->url[2]) ? $this->url[2] . "Ajax" : '',
'params' => $requestParams,
'database' => $this->database,
);
$this->primary = 'ajax';
}
return self::checkRoute();
}
/**
* Checks to see if a given route is valid
* @return bool
*/
private function checkRoute()
{
if ($this->controller != 'controllerajax') {
if (preg_match("#^[a-zA-Z_x7f-xff][a-zA-Z0-9_x7f-xff]*#", $this->controller)) {
try {
class_exists($this->controller);
} catch (Exception $e) {
$error = new ErrorHandler($this->database);
$error->invalidDispatch(debug_backtrace(), $this->controller);
$this->validRoute = false;
return false;
}
$reflection = new ReflectionClass($this->controller);
if (!class_exists($this->controller) || (!($reflection->hasMethod($this->method)))) {
$error = new ErrorHandler($this->database);
$error->invalidDispatch(debug_backtrace(), $this->controller, $this->method);
$this->validRoute = false;
return false;
}
} else {
$error = new ErrorHandler($this->database);
$error->invalidDispatch(debug_backtrace(), $this->controller);
$this->validRoute = false;
}
}
return true;
}
/**
* Dispatches the route to the requested controller
* and method.
* @return bool
*/
public function dispatch()
{
if ($this->validRoute == true) {
$dispatchedController = new $this->controller($this->database);
$method = $this->method;
return $dispatchedController->$method($this->params);
} else {
return false;
}
}
}
Solution
Your property doccomments are wrong. As they are you will only get that one long list of @vars for $url and the rest will remain empty. This defeats the purpose of the doccomments. The proper way is to put each doccomment above its respective property, like so:
protected
/** @var array Request URL */
$url,
/** @var array|string Controller Class Name */
$controller,
//etc...
;
You may also want to revisit those properties. First, unless this class is being extended, then these properties should be private, not protected. Second, most of these properties are doing entirely too much. Its not uncommon for a method to return multiple types, usually just one or two similar types; But a property should normally have just one. The way these are makes me think they are trying to do too much. For instance, $validRoute should probably only be a boolean, especially given the name.
Using whitespace to your advantage is good practice, especially if it helps with legibility. However, there is such a thing as too much. When aligning variable definitions, you should do so in groups and only to the extreme of the longest. I’m not sure what you were extending to, but it appeared to be beyond your longest definition. You should also be consistent. While scanning your code I at first didn’t realize that $requestParams was a variable because it was not aligned with the others. This is usually the main reason for such an alignment, to easily tell at a glance where your variables are.
$this->database = $database;
$this->url = explode('/', trim($_SERVER['REQUEST_URI'], '/'));
$this->validRoute = true;
$requestParams = $_REQUEST;
Now, the idea behind injecting parameters into your methods is to avoid tight coupling and allow for sharing information. This goes hand in hand with the Law of Demeter (LoD), which states that functions/methods should only know as much as is necessary to get its job done. However, all of the $_SERVER, $_REQUEST and $GLOBALS arrays are going against this. Especially since you have injected $_SERVER[ 'REQUEST_URI' ] as the $params parameter already, though you’re not using it. Avoid tight coupling and just inject what you need.
You can define the default value for $validRoute when you initially declare the property. Since it has a static value and is unused in the constructor there is no need to assign a value to it there. You wont even have to change the format of your properties. You can assign values to your properties while listing them.
protected
$url,
//etc...
$validRoute = TRUE,
$primary,
//etc...
;
If you have an array, whose individual parts make up different elements, you can use list() to define each part and avoid having to manually declare individual array elements. This is especially helpful because it avoids the use of magic numbers, which can be very confusing and detrimental to your code’s health. To ensure that you don’t get any “out of bounds” errors, you should pad your array before trying to list its elements.
array_pad( $this->url, 3, FALSE );
list( $request, $method, $params ) = $this->url;
if( $request != 'ajax' ) {
If you can avoid using “not” in your arguments, you should do so. Reverse the logic if necessary. This helps with legibility.
if( $this->url[ 0 ] == 'ajax' ) {
//etc...
$this->controller = empty( $this->url[ 0 ] ) ? 'controller\home' : 'controller\' . $this->url[ 0 ];
Ternary statements are nice, but you should only use them when they don’t hinder legibility. This means they should be short (less than 80 characters, including indentation), and simple (not nested or overly complex). If it violates either of these rules of thumb, you should probably consider using a regular if/else statement. Sometimes ternary statements aren’t even necessary. For example:
$var = $true ? TRUE : FALSE;//unnecessary
$var = $true;
//or
$this->params = empty( $this->url[ 2 ] ) ? $_REQUEST : $_REQUEST;//redundant
$this->params = $_REQUEST;
So, I’m assuming this $GLOBALS array is somehow associated with an actual global. It isn’t returned or used as far as I can tell, so this seems to suggest my assumption is correct. Globals are evil. Especially in a OOP environment where properties accomplish the same thing. You should never, and I DO mean NEVER, need a global. They are completely and utterly useless, no matter the circumstances. If you forgot what they were right now, you would never notice. I strongly urge you to remove this from your code.
Let’s look at one of the key OOP Principles: “Don’t Repeat Yourself” (DRY). As the name implies, your code should not repeat itself. This can even be as simple as reusing a value for a variable. For example:
$this->controller = 'controller\';
//etc...
$this->controller .= empty( $this->url[ 0 ] ) ? 'home' : $this->url[ 0 ];
There are only two reasons I can think of to use self::* instead of $this->*. The first is if your class is static, which you should avoid and doesn’t appear to be the case here. The second is if you are wanting to use a specific version of an inherited method. If the method is private, then it can’t be inherited and therefore will always be the same. Neither of these appears to be the case here, so, unless I’m missing something, you should just use $this->*.
return $this->checkRoute();
Besides the improvements I mentioned above your constructor is only very slightly violating the Single Responsibility Principle. This principle states that your functions/methods should do just one thing and not be concerned with anything else. Usually this principle is also combined with LoD so that the task can be reused without being tightly coupled. At most I think there are three things being done here. The first is instantiating the class, which is fine, this is the constructor after all. The second is breaking down the URL, which should probably be moved into its own method. And the third is checking the route, which is already in its own method. Just moving that second task to its own method is a good start, but you might also consider separating the last two tasks from the constructor by manually calling on them. Having your constructor do this automatically is fine if this will ALWAYS be necessary, but if you can think of even a single reason why it might not, then manually doing these tasks might be better.
So, next method. There are two key principles being violated here. The easiest to see is the Arrow Anti-Pattern. This principle illustrates code that is heavily indented to come to points, though just the heavy or unnecessary indentation is necessary for a violation, not the shape. The easiest way to avoid violating this principle is to return early whenever possible. This usually makes else and nested if/else statements unnecessary. For example:
if( $this->controller == 'controllerajax' ) {
return TRUE;
}
//etc...
The second principle is DRY again. Above I showed a very simple, but subtle, violation. This one is a bit more standard. You have chunks of code that are being repeated. In this case we can incorporate the use of a new method to report our errors. I’m using FALSE as the default value for our method, but you can use whatever you currently are.
private function reportError( $backtrace, $method = FALSE ) {
$error = new ErrorHandler( $this->database );
$error->invalidDispatch( $backtrace, $this->controller, $method );
$this->validRoute = FALSE;
}
The return value appears to be very closely linked to the $validRoute, so you can just return $validRoute to avoid the repetition.
return $this->validRoute = FALSE;
There is no need to explicitly check for a specific value unless you are specifically looking for that value, in which case you should then use the absolute === comparison rather than a loose == one. PHP automatically converts types for comparison.
if( $this->validRoute == TRUE ) {//redundant
if( $this->validRoute ) {//best option in this situation
//only true if $validRoute is a boolean true
//only necessary if specifically looking for this value
//unnecessary here as we are checking a protected boolean property
//we can assume its either TRUE/FALSE and use the above method instead
if( $this->validRoute === TRUE ) {
Since you have returned early, there is no need for the else statement. This just forces unnecessary indentation.
return $dispatchedController->$method($this->params);
} //else {//unnecessary
return false;