8
\$\begingroup\$

I made a simple routing class that I like to get reviews for, mostly because I do not know how to make it SOLID, since I even made this class separate from the HTTP response/request for the sake of SRP.

All this class does is route-specific, which seems like solid enough to me.

The actual library can be found at fastpress/framework for more reference.

Here is an example of how to use it separately from the framework:

require 'path/to/router.php'
use Somenamespace\Router as route; 
$route = new Route; 

// get request
$route->get('/', function(){ 
   echo 'Hello'; 
});

// get with regex named params 
$route->get('/user/{:name}/{:id}', function($name, $id){
   echo "name: $name id: $id";
});

// get with vanilla regex
$route->get('/user/[a-z]+', function(){}); 

// Rest-like API
$route->put('/foo', function(){});
$route->delete('/foo', function(){});
$route->post('/foo', function(){});

// MVC example. 
$route->get('/mvc', 'UserController@index'); 

NOTE: In the case of that last MVC example, where the handler is a string with "controller@index" and since for some reason, it did not seem right to me that a routing class should .. in this case:

  • check if controller file / class exists,
  • check if methods exists in controller class
  • instantiate the controller and inject the argument in the method

All inside a routing class, I just made the router return an array containing all these details for other class to take over and instantiate and load anything MVC-specific.

Here is the line in the router that does that:

if(is_string($routerPath) && strpos($routerPath, '@')){
    list($ctrl, $method) = explode('@', $routerPath); 
    return ['controller' => $ctrl, 'method' => $method, 'args' => $args];
}

Aside from that, the actual router class looks like this.

<?php

class Router{

protected $routes = [
    'GET'    => [],
    'POST'   => [],
    'ANY'    => [],
    'PUT'    => [],
    'DELETE' => [],   
];

public $patterns = [
    ':any'  => '.*',
    ':id'   => '[0-9]+',
    ':slug' => '[a-z\-]+',
    ':name' => '[a-zA-Z]+',
];

const REGVAL = '/({:.+?})/';    

public function any($path, $handler){
    $this->addRoute('ANY', $path, $handler);
}

public function get($path, $handler){
    $this->addRoute('GET', $path, $handler);
}

public function post($path, $handler){
    $this->addRoute('POST', $path, $handler);
}

public function put($path, $handler){
    $this->addRoute('PUT', $path, $handler);
}

public function delete($path, $handler){
    $this->addRoute('DELETE', $path, $handler);
}

protected function addRoute($method, $path, $handler){
    array_push($this->routes[$method], [$path => $handler]);
}

public function match(array $server = [], array $post){
    $requestMethod = $server['REQUEST_METHOD'];
    $requestUri    = $server['REQUEST_URI'];

    $restMethod = $this->getRestfullMethod($post); 

    #@TODO: Implement REST method. 

    if (!$restMethod && !in_array($requestMethod, array_keys($this->routes))) {
        return FALSE;
    }

    $method = $restMethod ?: $requestMethod;

    foreach ($this->routes[$method]  as $resource) {

        $args    = []; 
        $route   = key($resource); 
        $handler = reset($resource);

        if(preg_match(self::REGVAL, $route)){
            list($args, $uri, $route) = $this->parseRegexRoute($requestUri, $route);  
        }

        if(!preg_match("#^$route$#", $requestUri)){
            unset($this->routes[$method]);
            continue ;
        }

        if(is_string($handler) && strpos($handler, '@')){
            list($ctrl, $method) = explode('@', $handler); 
            return ['controller' => $ctrl, 'method' => $method, 'args' => $args];
        }

        if(empty($args)){
            return $handler(); 
        }

        #TODO: pass app by func array_push($args, $this);
         return call_user_func_array($handler, $args);

      }

      header('HTTP/1.1 404');
 }

protected function getRestfullMethod($postVar){
    if(array_key_exists('_method', $postVar)){
        if(in_array($method, array_keys($this->routes))){
            return $method;
        }
    }
} 

protected function parseRegexRoute($requestUri, $resource){
    $route = preg_replace_callback(self::REGVAL, function($matches) {
        $patterns = $this->patterns; 
        $matches[0] = str_replace(['{', '}'], '', $matches[0]);

        if(in_array($matches[0], array_keys($patterns))){                       
            return  $patterns[$matches[0]];
        }

    }, $resource);


    $regUri = explode('/', $resource); 

    $args = array_diff(
                array_replace($regUri, 
                explode('/', $requestUri)
            ), $regUri
        );  

    return [array_values($args), $resource, $route]; 
}
}

I would like to know how to make this solid, and if you were to remove some code to do so, please state your reason, not because symfony/laravel/xyz does so. Explain (if you can) why that code belongs separately from the class.

It also goes without saying that if you see any improvement, namely on speed or performance, let me know.

\$\endgroup\$
  • \$\begingroup\$ Are you invoking it with something like this?.. $route->match($_SERVER, $_POST); \$\endgroup\$ – Nigel Alderton Apr 5 '16 at 16:43
  • \$\begingroup\$ @NigelAlderton Nope. That is just to check if the current url from $_SERVER matches the regex pattern. The $_POST is being passed to match() so I can check if the request is RESTFul, or there are PUT, DELETE, UPDATE methods in post \$\endgroup\$ – samayo Apr 8 '16 at 12:55
  • \$\begingroup\$ Good question why no answer... \$\endgroup\$ – Timothy Coetzee Mar 27 '17 at 4:31
  • \$\begingroup\$ >[Composer\Downloader\TransportException] The "api.github.com/repos/fastpress/framework/zipball/… ed75c0" file could not be downloaded (HTTP/1.1 302 Found) \$\endgroup\$ – Anonymous Nov 16 '17 at 19:43
1
\$\begingroup\$

If we define SOLID as it is stated on Wikipedia:

Then several improvements are possible:

  1. Split the class into a Matcher and a Container class, to separate responsibilities
  2. Avoid protected methods and public properties, to keep the class "closed"
  3. Define an interface, for easier extendability
  4. Remove low-level dependencies (like sending 404 headers and using values from $_SERVER) outside of the class

Let me explain why...

Single-responsibility principle

A class should only have a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.

Currently the class has three separate responsibilities:

  1. Store routes
  2. Match routes
  3. Send a 404 header

At the very least, there should be 2 classes, one that stores the given routes and one that matches against the stored routes. This would also make the class more open to extension. Which leads us to...

Open–closed principle

"Software entities ... should be open for extension, but closed for modification."

Before I continue, an obvious improvement would be to make the public $patterns property private. The same goes for the protected methods.

For the sake of argument, lets say we want to create a router for CLI calls. With the current code that is not possible at all. An entirely new class would need to be created.

However, if the code was split into a store and a matcher, the store could be used as-is, and only the matching part would need to be (partially) changed.

Especially if an interface was created stating expected contract both classes should adhere to...

Liskov substitution principle

"Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program."

One reason to suggest another use-case is not because I think it is very likely that a CLI router will be needed, but because it offers a concrete example as counterpoint to having just one use-case. This leads to thinking about code in a way that allows exchanging one part of code with another, without the need for changes across other parts of your code-base.

This should also help you in creating those interface I mentioned before based on responsibility (or purpose) rather than on technical descriptions.

Which leads us to the...

Interface segregation principle

"Many client-specific interfaces are better than one general-purpose interface."

As always, the world we live in changes. New things are invented and subsequently need to be supported by your code. The example of needing a CLI router is purposefully far-fetched. But what about WebSockets? You would still need a URL but HTTP Methods now no longer make sense. The mechanism of a WebSocket storage/container class would be different from an HTTP class. But the Matcher class could still be the same... In fact, the Matcher class wouldn't care about how routes got in to the container.

It would only be concerned with how it could get routes out. So you can have two classes, one for HTTP and one for WSS. Both of these classes would implement the same interface for the Matcher but they can each have their own interface to facilitate their specific (client) responsibility.

Of course, in order to do this, the code needs to be thought of (i.e. designed) from an abstract perspective but implemented from a concrete perspective, because of the...

Dependency inversion principle

One should "depend upon abstractions, [not] concretions."

One very simple example of this in the code is the match method requiring a $server parameter, only to extract REQUEST_METHOD REQUEST_URI.

The obvious suggestion would be to pass those two values in separately. Why?

Because in its current state, the class needs to know details about the contents of $server. But what if those change? Or need extending? (Think of the CLI or WSS examples mentioned before).

By asking for the request method and uri explicitly, the "contract" for the class is more apparent. And because of this, if you were to need/create a class that does not need the request method (for instance WSS), it would immediately be apparent from the side of the code calling match instead of needing to look in the match code itself.

A similar case can be made even without an example such as WSS or CLI extentions. Just thing about the request methods for a moment... The code currently supports GET, POST, ANY, PUT, and DELETE.

But what about HEAD, CONNECT, OPTIONS, TRACE, and PATCH?

Do they still play well with ANY? Do they need different logic? And do you really want to add another array to your code for each? That alone should give you enough to think about how the code might not be as SOLID as you think.

| improve this answer | |
\$\endgroup\$

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.