patternphpMinor
Router for personal use
Viewed 0 times
userouterforpersonal
Problem
This is router I created for use, however, I wish to refactor this code and make this more robust. I would really appreciate anyone who can:
-
Point out how the code could be made more efficient.
Implementation:
Source Code:
```
checkWhetherParametersAreRequired($uri);
if($result != false) {
$this->routes[] = [$uri, $class, $method, $result];
return true;
}
$this->routes[] = [$uri, $class, $method];
return true;
}
public function checkWhetherParametersAreRequired($uri)
{
if(preg_match_all($this->patternForURIs, $uri, $matches)) {
return $matches[0];
}
return false;
}
public function runRouter()
{
$routesWhichExpectParameters = array();
foreach($this->routes as $route) {
if(isset($route[3])) {
$routesWhichExpectParameters[] = $route;
}
if($route[0] === $_SERVER["REQUEST_URI"]) {
return $route;
}
}
foreach($routesWhichExpectParameters as $route) {
$route[0] = ltrim($route[0], '/');
$route[0] = explode('/', $route[0]);
$partsToConfirm = count($route[0]);
$requestURI =
- Point out how to make this code more robust, clean and flexible in terms of reusability where the user of this module can easily alter the configuration, etc to his/her needs without possibly altering the source code.
- Point out where code can be refactored and possibly how.
-
Point out how the code could be made more efficient.
- Point out places where I have breached any principles of OOP.
Implementation:
$router = new Router\Router();
/**
* The arguments for the method createRoute:
* First Argument - URI
* Second Argument - Namespace or Classname of which the method will be executed.
* Third Argument - The method to be run in the specified namespace/class in the second argument.
*/
$router->createRoute('/', 'PageController', 'showHome');
$result = $router->runRouter();Source Code:
```
checkWhetherParametersAreRequired($uri);
if($result != false) {
$this->routes[] = [$uri, $class, $method, $result];
return true;
}
$this->routes[] = [$uri, $class, $method];
return true;
}
public function checkWhetherParametersAreRequired($uri)
{
if(preg_match_all($this->patternForURIs, $uri, $matches)) {
return $matches[0];
}
return false;
}
public function runRouter()
{
$routesWhichExpectParameters = array();
foreach($this->routes as $route) {
if(isset($route[3])) {
$routesWhichExpectParameters[] = $route;
}
if($route[0] === $_SERVER["REQUEST_URI"]) {
return $route;
}
}
foreach($routesWhichExpectParameters as $route) {
$route[0] = ltrim($route[0], '/');
$route[0] = explode('/', $route[0]);
$partsToConfirm = count($route[0]);
$requestURI =
Solution
You need to decide on the Domain Objects that represent concepts in your current model.
A
So, now you know that you want to get back some
The object API I would then expect to be using would be:
Somewhere in the
Don't expect the user to remember to call
In the context of routing itself, you'll likely want something called a Resolver. That is, something you pass a
It looks like you're trying to design some generic router, but it is doing too many things. Focus on making the following seperately:
Separate the actions out that you want to do, contextually, and you'll achieve better SoC.
Also, you shouldn't be accessing any superglobals (
A
Route object should encapsulate the data to do with a route. If you look at Symfony's Route you'll see that it is simply an object with some properties, getters and setters, and that's about it (apart from a few select helper functions). This is also known as an Entity, the end object that you get back.So, now you know that you want to get back some
Route entities (@return Route[] in phpdoc), you should:- Create a
Routeobject which merely represents a route
- Take a look at the Factory Pattern and create a factory for these
The object API I would then expect to be using would be:
$router = new Router(new RouteFactory);
/** @var Route $route */
$route = $router->createRoute('/', 'PageController', 'showHome');Somewhere in the
Router, it would call RouteFactory::create($param1, $param2 ...) to actually make a Route object.Don't expect the user to remember to call
createRoute() followed by runRoute(). If one can't be run before the other, then it looks like you're trying to implement the builder creational pattern (which is for optional parameters). Your createRoute() method should return a Route object.In the context of routing itself, you'll likely want something called a Resolver. That is, something you pass a
Route to, and it instantiates the thing you are routing to. In an 'MVC' framework, you have a ControllerResolver - so you would create a Route, call ControllerResolver::resolve(Route $route) and that would contain the logic to decide what controller to create based on the contents of the Route object (so it'd do things like class_exists($controllerName) etc.It looks like you're trying to design some generic router, but it is doing too many things. Focus on making the following seperately:
- An object representing your route (
Route)
- An object responsible for creating a route from
$_POSTetc (RouteFactory::createFromGlobals($_POST)(or similar))
- An object responsible for taking your
Routeand doing what you want to do with it (ControllerResolver(or similar))
Separate the actions out that you want to do, contextually, and you'll achieve better SoC.
Also, you shouldn't be accessing any superglobals (
$_*) within this class, you should pass them in instead. This means that during testing you fake (or 'mock') the data you pass in with relative ease and test that your object still functions as you expect over different scenarios.Code Snippets
$router = new Router(new RouteFactory);
/** @var Route $route */
$route = $router->createRoute('/', 'PageController', 'showHome');Context
StackExchange Code Review Q#87365, answer score: 8
Revisions (0)
No revisions yet.