5
\$\begingroup\$

I wrote a router class which accepts a URL and calls the appropriate controller method based on it.

I'm a little worried about the amount of dependencies it has (eg ErrorController, RequestMethod, etc), because it might make reusing it in other projects difficult.

It feels especially bad because a lot of the dependencies are static (and there might be even more in the future; eg if I add support for multiple languages). On the other hand, I also did not want to pass too many arguments to the router, and I'm not sure if it makes sense to create new objects every time for each of the other classes, and I also did not want them cluttering up my private fields (although that might be best?).

I don't write all that much in PHP, so I'm happy about any other comments regarding code quality as well.

<?php
namespace MyProject;

use MyProject\controller\ErrorController;
use MyProject\inc\String;
use MyProject\model\RequestMethod;
use MyProject\Connection;
use MyProject\model\exceptions\UnknownRequest;

class Router {

    private $connection;

    function __construct(Connection $connection, $requestUrl, $routes) {
        $this->connection = $connection;        
        try {
            $serverRequestMethod = RequestMethod::parseRequestMethod();
        } catch(UnknownRequest $unknown) {
            ErrorController::notFound("illegal request method", $unknown);
        }
        $url = $this->cleanURL($requestUrl);
        $this->route($url, $serverRequestMethod, $this->routeIdToRegex($routes));
    }

    /**
     * calls a controller method based on the request url and defined routes.
     * 
     * @param string    $url         the requested url, eg /user/5
     * @param string    $requestType POST, GET, PUT, or DELETE
     * @param array     $routes      a whitelist of allowed urls + request types mapped to a controller + method. Example entry: "GET /" => "UserController.listAll",
     * @return type|null           return-value of called controller method. If url is not defined in the routes, ErrorController::notFound will be called.
     */
    private function route($url, $requestType, $routes) {
        $url = $this->removeQuery($url);
        foreach ($routes as $route => $controllerAndMethod) {
            if (String::startsWith($route, $requestType) && preg_match("~^" . $this->cleanRoute($requestType, $route) . "$~", $url, $arguments)) {
                $arguments = array_slice($arguments, 1);
                return $this->executeRoute($controllerAndMethod, $arguments);
            }
        }
        ErrorController::notFound("$requestType $url ");
    }

    /**
     * calls a controller and method with arguments.
     * <p>
     * The connection will always be passed as the first argument.
     * 
     * @param string $controllerAndMethod controller and method to be called in the form "Controller.method"
     * @param array $arguments additional arguments to pass to called method
     * @return type|null return-value of called controller method. If url is not defined in the routes, ErrorController::notFound will be called.
     */
    private function executeRoute($controllerAndMethod, $arguments) {
        list($class, $method) = explode(".", $controllerAndMethod);
        if (!method_exists(NAMESPACE_CONTROLLER . $class, $method)) {
            ErrorController::internalServerError("called undefined method $class :: $method");
        }
        return call_user_func_array(
                array(NAMESPACE_CONTROLLER . $class, $method),
                array_merge(array($this->connection), $arguments));
    }

    /**
     * removes script name and trailing /. If url is then empty, it will return /.
     * 
     * Eg: /your-dir/index.php/user/test/34/bla/ -> /user/test/34/bla
     * Eg: /your-dir/index.php -> /
     * 
     * @param string    $url    the url
     * @return string           cleaned url
     */
    private function cleanURL($url) {
        $urlToRemove = str_replace(BASE_DIR, "", $_SERVER["SCRIPT_FILENAME"]);
        $url = str_replace($urlToRemove, "", $url);
        return String::equals($url, '/') || String::equals($url, '') ? '/' : rtrim($url, '/');
    }

    /**
     * removes request type and whitespace from route.
     * 
     * Eg: "GET     /user/[id]" -> "/user/[id]"
     * 
     * @param string $requestType   POST, GET, PUT, or DELETE
     * @param string $route         the route
     * @return string               cleaned route
     */
    private function cleanRoute($requestType, $route) {
        return trim(str_replace($requestType, "", $route));
    }

    /**
     * removes the query string from a url.
     * 
     * Eg /your-dir/index.php/user/5?limit=12 -> /your-dir/index.php/user/5
     * 
     * @param string $url   the url
     * @return string       the url without the query string
     */
    private function removeQuery($url) {
        if (String::contains($url, '?')) {
            return substr($url, 0, strpos($url, '?'));
        }
        return $url;
    }

    /**
     * replaces the keys in an array according to internally defined rules, and removes trailing /

      In practice, it can be used to transform eg

      $routes = array(
      "/user/[id]/"             => "UserController.show",
      "/user"                       => "UserController.list",
      "/user/[id]/[string]"         => "UserController.show",
      );

      to

      $routes = array(
      "/user/(\d+)"             => "UserController.show",
      "/user"                       => "UserController.list",
      "/user/(\d+)/(\w+)"       => "UserController.show",
      );
     * 
     * @param array $routes 
     * @return array
     */
    private function routeIdToRegex($routes) {
        $routeIdToRegexMap = array(
            "[id]" => "(\d+)",
            "[string]" => "(\w+)",
        );

        foreach ($routeIdToRegexMap as $routeId => $regex) {
            foreach ($routes as $routeKey => $routeValue) {
                if (String::contains($routeKey, $routeId)) { // only replace id if it exists, otherwise entry would be deleted
                    $updatedRouteKey = rtrim(str_replace($routeId, $regex, $routeKey), '/');
                    $routes[$updatedRouteKey] = $routeValue;
                    unset($routes[$routeKey]);
                }
            }
        }
        return $routes;
    }
}

It can then be used like this:

/**
 * note: none of the routes may contain "?" in any parts.
 * this means that "/user/te?st/[id]/" would not be allowed. 
 * Normal GET arguments still work though, so routing doesn't forbid 
 *     localhost/your-dir/index.php/user/1?litmit=123
 * 
 * @see MyProject\model\RequestMethod for PUT/DELETE support
 */
$routes = array(
    "GET    /"                => "UserController.listAll",
    //
    "PUT    /user/[id]"       => "UserController.update",
    "DELETE /user/[id]"       => "UserController.delete",
    "GET    /user/[id]"       => "UserController.show",
    "GET    /user"            => "UserController.listAll",
    "POST   /user"            => "UserController.create",
);
try {
    $connection = new MyProject\Connection();
} catch (\PDOException $e) {
    MyProject\controller\ErrorController::internalServerError('Connection to database failed', $e);
}
$router = new MyProject\Router($connection, $_SERVER["REQUEST_URI"], $routes);

I'm also a little worried about usability, as the user has to look up how to structure the routes array. I thought about adding a addRoute(requestMethod, route, controllerClass, method) method, which would mean that the router could be used like this:

$router = new MyProject\Router($connection, $_SERVER["REQUEST_URI"]);
$router->addRoute("GET", "/user/[id]", "UserController", "listAll");
$router->route();

But I think that that might be less readable, so I'm unsure.

\$\endgroup\$
  • \$\begingroup\$ why don't you have a look at the routing components of the big ones; of Symfony or Zend? Or in other words: do you want to do some exercises or do the implementations not satisfy your requirements? \$\endgroup\$ – Aitch Dec 24 '15 at 3:17
  • \$\begingroup\$ @Aitch the routing of symfony looked a lot more complex than what I wanted (and it also has a lot of dependencies outside the routing component, so it would be hard to extract from symfony). But yes, this is just an exercise, I'm currently not planning on using it in real-world applications. \$\endgroup\$ – tim Dec 24 '15 at 7:44
  • \$\begingroup\$ I would not pass your array in the ctor, because you also have to provide any addRoute methods for different independent components to add their routing. I also would not pass the current url, because think of a Router like url is input and a controller callable or whatever is output. And don't pass a db connection object. A router has nothing to do with a db connection, unless you build something like the CMF router which saves everythin in the db (github.com/symfony-cmf/Routing) \$\endgroup\$ – Aitch Dec 25 '15 at 11:44
  • \$\begingroup\$ Rather than guess what might be cool, just write a router test and if your test looks nice (which is production code), then implement your router. \$\endgroup\$ – Aitch Dec 25 '15 at 11:48
  • \$\begingroup\$ @Aitch I passed the db connection, as I want to use one connection across the application and am not yet sure if I need it before the routing as well (eg for authorization or authentication). The only other way I could think of would be a singleton, which also doesn't seem ideal. If you wouldn't pass the current URL, what else would you pass? Or would the router retrieve the URL itself? And sure, just testing what will work well is one approach, but I like to plan ahead, as just writing what seems usable sometimes results in complete rewrites when it turns out that it isn't the best approach. \$\endgroup\$ – tim Dec 28 '15 at 11:40
2
\$\begingroup\$

Working with methods is easier than configuration arrays. Configuring routes through method calls gives some overhead. If you do not feel it then there is nothing to worry about. If you do however, you could even create a code configurator that is called only once, to obtain configuration array, and result is cached. If decent IDE is in play, then adding route with fluent style configurator would be a breeze. You do not need to know configuration array structure and it can even change at background. That structure is used by another object some where else and it is part of that implementation detail.

Decouple your concerns by implementing strategies. Instead of passing all controller dependencies via router use dependency injection. Using those two principles alone gives you more flexibility.

You can create specific contract and implementation for it that calls static method for you if you must have them.

I hope this rambling puts you on some track. Please, feel free to ask questions! (:

p.s. hope that this is not considered as tumbleweed

\$\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.