3
\$\begingroup\$

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 . '\/?$';

            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:

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

$router = new Router($routes, $path, $method);
$router->load();
  1. Am I utilizing dependency injection correctly?
  2. Are there any parts of my class that I should decouple?
  3. Is there anything I could improve in my code when it comes to OO PHP (and MVC)?
\$\endgroup\$
3
\$\begingroup\$

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 request parameters, but processing and validating those parameters falls, in turn, under the responsibility of a Request object.
On more details on this matter, you can check this answer of mine, which reviews a Request object.

So: the answer to this question is: yes. Your code is not SOLID, as it violates the single responsibility principle by acting as a dispatcher, router, and in some ways request object factory.
Well, it doesn't function as a request object just yet, but it passes the request params to the classes it instantiates. Chances are that, in time, you'll be adding some validation methods to this class, or create a new object (Params or something), and have your Route class create those instances as it passes them to the classes load creates...

Anything I could improve in my code when it comes to OOP (and MVC)?
At this point: you're going to have to work on this some more: you show one class that is quite small. There are signs of promise, but I can't honestly assess your OO skills or correctness of your understanding of OO paradigms and MVC pattern.

The only thing I really can and will criticize on is your coding style. Now you may think this is a simple matter of personal preference, but really: it's important to follow certain standards! Such standards exist: Python has PEP, PHP has FIG, their site: PHP-FIG.
One of the things I've noticed is your tendency of prepending every non-public property/method with an underscore. This practice stems from the days where PHP didn't (yet) have access modifiers - back in the days of PHP4.
Read through the PSR documents, and conform to those standards as much as you can.
I try to conform to everything in there, except for my Allman brackets. I know, I need to practice what I preach, but a single deviation from the standards is not that bad, IMO, especially if you have a good IDE, simply re-format your code before commiting to GIT, and nobody need ever know :).

On the last, I have one thing that is rather worrying to me: You create an instance of a given class, and pass the request params to either a GET or POST method. That sounds to me like you have 1 class for each possible URL. That is absolutely not the way to go

Before you start, always see what is already out there: Symfony2, Zend, CI, Cake... are all MVC frameworks that have been around for a long time. They are all open-source, and have a routing component. Just check the source code of those projects, and see if you can't get any ideas from that

Remember: A good programmer is a lazy one. Don't invent something that already exists.

\$\endgroup\$
  • \$\begingroup\$ I take my hat off to you for giving me such a great and constructive answer. So, I'm starting from the top and will ask questions accordingly as I get to the bottom. My config is in fact an array that holds all the setting information, I just processed it so that I could call it like an object. I checked CodeIgniter, and they use an array too for that. Should I leave the dependency injection (Array $routes) as is? \$\endgroup\$ – Kid Diamond Apr 29 '14 at 11:03
  • 1
    \$\begingroup\$ @KidDiamond: That really depends on what kind of code you are writing. If your aim is to build your own framework, then I wouldn't leave it as an array, I'd create an object. an array type-hint allows users to pass range(1,4) to your constructor, resulting in regex's like /^0\/?$/, which isn't what you want: you expect the array to be an associative array, where the keys are uri's. An array simply allows too much room for error (as in: invalid data being passed and what not) \$\endgroup\$ – Elias Van Ootegem Apr 29 '14 at 11:10

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.