Code Review Asked by samayo on January 3, 2021
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 SomenamespaceRouter 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:
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.
If we define SOLID as it is stated on Wikipedia:
Then several improvements are possible:
protected
methods and public
properties, to keep the class "closed"$_SERVER
) outside of the classLet me explain why...
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:
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...
"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...
"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...
"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...
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.
Answered by Potherca on January 3, 2021
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP