4
\$\begingroup\$

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 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:

<?php
namespace Router;

class Router
{
    private $routes = array();
    private $patternForURIs = '/\{[a-zA-Z0-9]*\}/';

    public function createRoute($uri, $class, $method)
    {
        $result = $this->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 = ltrim($_SERVER["REQUEST_URI"], '/');
            $requestURI = explode('/', $requestURI);
            $partsAvailable = count($requestURI);

            if($partsToConfirm != $partsAvailable) {
                continue;
            }

            $confirmedParts = 0;

            $numberOfParametersToDetect = count($route[3]);
            $numberOfParametersDetected = 0;
            $detectedParameters = array();

            for($iterator = 0; $iterator < $partsToConfirm; $iterator++) {
                if($route[0][$iterator] == $requestURI[$iterator]) {
                    $confirmedParts++;
                    continue;
                } else if(preg_match($this->patternForURIs, $route[0][$iterator])) {
                    $confirmedParts++;
                    $detectedParameters[] = array(
                        $route[3][$numberOfParametersDetected] => $requestURI[$iterator]
                    );

                    $numberOfParametersDetected++;
                    continue;
                }
            }

            if(($numberOfParametersDetected == $numberOfParametersToDetect) && ($partsToConfirm == $confirmedParts)) {
                $route[4] = $detectedParameters;
                return $route;
                break;
            }
        }
        return false;
    }
}
\$\endgroup\$
8
  • \$\begingroup\$ Will award bounty on the 5th. \$\endgroup\$ – Abandoned Account Apr 30 '15 at 17:15
  • \$\begingroup\$ Waiting for more answers \$\endgroup\$ – Abandoned Account Apr 30 '15 at 17:15
  • \$\begingroup\$ In order to help make the code more "flexible" we need to understand in what way. For instance, I would consider allowing anonymous functions or closures instead of only allowing class methods… but that may hurt performance. \$\endgroup\$ – Levi Morrison May 2 '15 at 8:00
  • \$\begingroup\$ @LeviMorrison updated question. \$\endgroup\$ – Abandoned Account May 2 '15 at 8:04
  • \$\begingroup\$ I don't know PHP. Is the first line $router = new Router\Router(); and not just $router = new Router(); ? \$\endgroup\$ – bhathiya-perera May 2 '15 at 8:59
8
+25
\$\begingroup\$

You need to decide on the Domain Objects that represent concepts in your current model.

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 Route object 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 $_POST etc (RouteFactory::createFromGlobals($_POST) (or similar))
  • An object responsible for taking your Route and 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.

\$\endgroup\$
5
  • \$\begingroup\$ Its a quite good answer but I will wait more. Upvoted. \$\endgroup\$ – Abandoned Account Apr 30 '15 at 7:10
  • \$\begingroup\$ Also, if you can add some mock code to explain it would be much easier for me to understand as I find it hard to grasp just raw english. \$\endgroup\$ – Abandoned Account Apr 30 '15 at 7:13
  • \$\begingroup\$ I believe that the responsibility to take the route and doing what you have to do with it is the responsibility of the user who uses the library \$\endgroup\$ – Abandoned Account Apr 30 '15 at 8:10
  • \$\begingroup\$ @HassanAlthaf It depends what the point of your router is. Is it to provide a route to the user? If so, that's fine... but if you're using it in some sort of framework then you won't have an end-user for that specific part of it. \$\endgroup\$ – James Apr 30 '15 at 9:14
  • \$\begingroup\$ Not a framework. I am not doing that part because the user may want to call the method statically or instantiate the class and then call the method via the instance. And they may also want to pass in some params. \$\endgroup\$ – Abandoned Account Apr 30 '15 at 9:16

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.