Router for MVC framework

The class routes URLs based on the domain.com/class/method/param/.. format. It also checks the request type (GET or POST) and calls the method name GET or POST from the identified class.

class Router {
    private $_routes,
            $_path,
            $_method,
            $_found;

    public function __construct(Array $routes, $path, $method) {
        krsort($routes);

        $this->_routes = $routes;
        $this->_path   = $path;
        $this->_method = strtoupper($method);
    }

    public function load() {
        foreach($this->_routes as $regex => $class) {
            $regex = str_replace('/', '\/', $regex);
            $regex = '^' . $regex . '\/?

I load it up in my entry file like so:

//url to class router
$routes = (Array) $config->routes;
$path   = $_SERVER['REQUEST_URI'];
$method = $_SERVER['REQUEST_METHOD'];

$router = new Router($routes, $path, $method);
$router->load();


Am I utilizing dependency injection correctly?



Are there any parts of my class that I should decouple?



Is there anything I could improve in my code when it comes to OO PHP (and MVC)?

;

            if(preg_match('/' . $regex . '/i', $this->_path, $params)) {
                $this->_found = true;

                $this->_classInstantiate($class, $params);

                break;
            }
        }

        if(!$this->_found) {
            throw new Exception('URL location not found: ' . $this->_path);
        }
    }

    private function _classInstantiate($class, $params) {
        if(class_exists($class)) {
            $obj = new $class($params);

            if(method_exists($obj, $this->_method)) {
                $method = $this->_method;

                $obj->$method();
            } else {
                throw new BadMethodCallException('Method not found: ' . $this->_method);
            }
        } else {
            throw new Exception('Class not found: ' . $class);
        }
    }
}

I load it up in my entry file like so:

%%CODEBLOCK_1%%

• Am I utilizing dependency injection correctly?

• Are there any parts of my class that I should decouple?

• Is there anything I could improve in my code when it comes to OO PHP (and MVC)?

Let's look at your questions individually:

Am I utilizing dependency injection correctly?

Yes and no. You are, essentially, injecting the routes. That's fine. But you extract them from a $config object. If you are working out some MVC (micro-) framework, then chances are that config object is an instance of a specific class.

As your code-base grows, so will the config, and chances are pretty good that the Route class will need more data from the config. Instead of passing an array to the constructor then, I'd simply pass the whole $config object, and type-hint that.

That way, when you only have to refactor the Route constructor (as opposed to all code that creates a new Route instance) if you decide to add to the config object, something the Route class needs. For example: a list of approved/registered classes that the load method can instantiate:

private $routes = null;
private $classMap = array();
public function __construct(Config $conf)
{
    $this->routes = (array) $conf->routes;
    foreach ($conf->classMap as $class => $paramData)
    {//paramData could contain which arguments are required
     //which are optional, or default values
        $this->classMap[$class] = array(
            'required' => isset($paramData['required']) ? $paramData['required'] : array(),
            'defaults' => isset($paramData['defaults']) ? $paramData['defaults'] : array()
        );
    }
}

The load method... There is a lot to say about this, but as it now stands, I have a couple of suggestions:

Why not group the $routes by method? Suppose you have 100 routes in total, 20 of which are POST routes, what good does looping through all 100 routes do?

You are not properly escaping the path regex. Suppose there is a question mark in the $regex string? By calling str_replace('/', '\/', $regex); you're also not quite doing what you thing you are doing. You're creating an escape sequence, escaping the forward slash, not the backslash you effectively need!

You want to escape the delimiter, so you should escape the backslash: use str_replace('/', '\\/', $regex);

However all this is pretty useless anyway, simply because there exists a ready-made PHP function that can escape a string that is going to be used in preg_match a lot better anyway:

$regex = preg_quote($regex, '/');

And instead of calling this function every time the load function is called: use a setter (or 2 setters) for the $this->routes property:

public function setRoutes(array $routes)
{
    //assume GET && POST keys, too:
    $this->routes = array(
        'GET' => array(),
        'POST' => array()
    );
    foreach ($this->routes as $method => $x)
    {//ignore $x
        if (!isset($routes[$method]))
            continue;//ignore. Be weary: case sensitive, a class would be useful here
        foreach ($routes[$method] as $regex => $class)
        {
            //optionally check $class is allowed by $this->classMap!
            $this->routes[$method][preg_quote($regex, '/')] = $class;
        }
    }
}

Now this is written off the top of my head, and a bit messy. What follows is messier still, but just so you know: you can write this as a one-liner, if you use / as the regex delimiter:

$this->routes = array(
    'GET'  => array_combine(
        array_map(
            'preg_quote',
            array_keys($routes['GET'])
        ),
        $routes['GET']
    ),//same for POST
);

If you decide to use a different delimiter:

$this->routes = array(
    'GET'  => array_combine(
        array_map(
            'preg_quote',
            array_keys($routes['GET']),
            array_fill(0, count($routes['GET']), '~')//~ is now your delimiter
        ),
        $routes['GET']
    ),
);

Now, the $method and $params arguments from the constructor are only really used in the load method. Why pass them to the constructor? Why not pass them to load, and make them optional? If ever you want to write tests for this class, surely it would be easier to be able to pass invalid/impossible requests to this class.

And even when not testing: Sometimes you want your controller to perform the actions defined in 2 distinct controller actions. By changing the route, and call load again, you could do just that.

Making them optional implies that calling load will have load fall back to the $_SERVER super-global. Even so, let me now tell you why you shouldn't refactor this Route class too much, by answering your next question:

Any parts of my class that I should decouple?

Perhaps you should first ask yourself what this class is? Its name, to me, suggests it is a router class. What is a router class's job? Surely, it is to parse, validate, generate URLS, and perhaps redirect.

The URL it extracts from a request can then be used by a dispatcher or front-controller to create an instance of a specific controller, and invoke methods.

A router can indeed check to see if there are some re

StackExchange Code Review Q#48396, answer score: 3