3
\$\begingroup\$

I had previously asked the question here.

The response was pretty much "wow this is bad".

So I learned everything I could and wrote what I believe to be better using TDD.

The strict requirement is that the url will be parsed as /controller/action/everything/else/is/a/parameter.

So here is what I wrote:

The router class

<?php

declare(strict_types=1);

namespace xxxx\xxxx\Router;

use xxxx\xxxx\Util\StorageUtils;

class Router{

    public array $defaultRoute;

    public array $notFoundRoute;

    public array $urlSegments;

    public $controller;

    public $action;

    public $parameters;

    public $controllerName;

    public $view;

    public function setDefaultRoute($controller, $action): void{
        $this->defaultRoute = [$controller, $action];
    }

    public function setNotFoundRoute($controller, $action): void{
        $this->notFoundRoute = [$controller, $action];
    }

    public function execute($url): void{
        $this->getUrlSegments($url)
            ->determineController()
            ->createController()
            ->determineParameters()
            ->determineAction()
            ->executeAction()
            ->determineView();
    }

    public function executeView(){
        $this->controller->initData();
        
        if($this->controller->data != null){
            extract($this->controller->data, EXTR_PREFIX_SAME, 'wddx');
        }

        if($this->view != null){
            $viewPath = StorageUtils::getFullPath($this->getViewFile($this->controllerName, $this->view));
        
            if($this->controller->useTemplate){
                $templatePath = StorageUtils::getFullPath($this->getViewFile('Template', $this->controller->template));

                require_once($templatePath);
            }else{
                require_once($viewPath);
            }
        }

        $this->controller->init();
    }

    public function executeAction(): self{
        $action = $this->action;
        $this->controller->$action($this->parameters);

        return $this;
    }

    public function getUrlSegments($url): self{
        $url = explode('/', ltrim($url,'/'));

        $this->urlSegments = $url;

        return $this;
    }

    public function determineController(): self{
        if(count($this->urlSegments) > 0){
            $controller = ucfirst($this->urlSegments[0]);

            if(empty($controller)){
                $this->controllerName = $this->defaultRoute[0];
                return $this;
            }

            if($this->controllerExist($controller)){
                $this->controllerName = $controller;
                return $this;
            }
        }

        $this->controllerName = $this->notFoundRoute[0];
        return $this;
    }

    public function createController(): self{
        $controllerName = $this->controllerName;
        $this->requireFile($this->getControllerFile($controllerName));
        $this->controller = new $controllerName();

        return $this;
    }

    public function determineParameters(): self{
        $this->parameters = array_slice($this->urlSegments, 2);

        return $this;
    }

    public function determineAction(): self{
        $action = 'index';

        if(count($this->urlSegments) > 1){
            if(strlen($this->urlSegments[1]) > 0){
                $action = $this->urlSegments[1];
            }
        }

        if(!$this->actionExist($action)){
            $this->controllerName = 'ErrorPages';
            $this->createController();
            $action = $this->notFoundRoute[1];
        }

        $this->action = $action;

        return $this;
    }

    public function determineView(): self{
        $this->view = $this->controller->view;

        if(!is_string($this->view)){
            $this->view = null;
        }

        if(!$this->viewExist($this->controllerName, $this->view) && $this->view != null){
            $this->controllerName = 'ErrorPages';
            $this->createController();
            $this->action = $this->notFoundRoute[1];

            $this->executeAction();

            $this->view = '404';
        }

        return $this;
    }

    public function controllerExist($controllerName): bool{
        return StorageUtils::fileExist(StorageUtils::getFullPath($this->getControllerFile($controllerName)));
    }

    public function actionExist($action): bool{
        return method_exists($this->controller, $action);
    }

    public function viewExist($controllerName, $view): bool{
        return StorageUtils::fileExist(StorageUtils::getFullPath($this->getViewFile($controllerName, $view)));
    }

    public function getControllerFile($controllerName): string{
        return "/src/Application/$controllerName/Controller/$controllerName.php";
    }

    public function getViewFile($controllerName, $viewName): string{
        return "/src/Application/$controllerName/View/$viewName.php";
    }

    private function requireFile($route): void{
        require_once(StorageUtils::getFullPath($route));
    }
}
?>

The corresponding test class

<?php

namespace xxxx\xxxx\Test;

use PHPUnit\Framework\TestCase;

use xxxx\xxxx\Router\Router;

class RouterTest extends TestCase{

    private $router;

    public function setUp(): void{
        $this->router = new Router();

        $this->router->setDefaultRoute('Home', 'index');
        $this->router->setNotFoundRoute('ErrorPages', 'notFound');
    }

    /**
     * @covers getUrlSegments
     */
    public function testGetUrlSegments(){
        $returnValue = $this->router->getUrlSegments('/test/index/111/222');

        $this->assertNotEmpty($this->router->urlSegments);
        $this->assertIsArray($this->router->urlSegments);
        $this->assertEquals('test', $this->router->urlSegments[0]);
        $this->assertEquals('index', $this->router->urlSegments[1]);
        $this->assertEquals('111', $this->router->urlSegments[2]);
        $this->assertEquals('222', $this->router->urlSegments[3]);

        $this->assertEquals(Router::class, $returnValue::class);
    }

    /**
     * @covers setDefaultRoute
     */
    public function testSetDefaultRoute(){
        $this->assertEquals('Home', $this->router->defaultRoute[0]);
        $this->assertEquals('index', $this->router->defaultRoute[1]);
    }

    /**
     * @covers setNotFoundRoute
     */
    public function testSetNotFoundRoute(){
        $this->assertEquals('ErrorPages', $this->router->notFoundRoute[0]);
        $this->assertEquals('notFound', $this->router->notFoundRoute[1]);
    }
    
    public function routeProvider(){
        return [
            ['/test', 'Test'],
            ['/test/sdf24', 'Test'],
            ['/home', 'Home'],
            ['/', 'Home'],
            ['', 'Home'],
            ['/home/', 'Home'],
            ['adafeadsdas', 'ErrorPages'],
            ['ad/asd/efwefd/qed', 'ErrorPages'],
            ['!@#@!#@', 'ErrorPages'],
            ['/!@@$2323', 'ErrorPages'],
            ['/.html', 'ErrorPages'],
            ['/someNonExistentController', 'ErrorPages'],
            ['/%', 'ErrorPages'],
            ['/@@$%$%%5E', 'ErrorPages']
        ];
    }

    /**
     * @covers determineController
     * @dataProvider routeProvider
     */
    public function testDetermineController($url, $expectedResult){
        $this->router->getUrlSegments($url)
            ->determineController();

        $this->assertEquals($expectedResult, $this->router->controllerName);
    }

    /**
     * @covers createController
     * @dataProvider routeProvider
     */
    public function testCreateController($url, $expected){
        $this->router->getUrlSegments($url)
            ->determineController()
            ->createController();

        $this->assertEquals($expected, $this->router->controller::class);
    }

    public function actionDataProvider(){
        return [
            ['/test', 'index'],
            ['/test/doesNotExist', 'notFound'],
            ['/test/test', 'test'],
            ['/test/!#$@#$ad', 'notFound'],
            ['/test/', 'index']
        ];
    }

    /**
     * @covers determineAction
     * @dataProvider actionDataProvider
     */
    public function testDetermineAction($url, $expected){
        $this->router->getUrlSegments($url)
            ->determineController()
            ->createController()
            ->determineAction();

        $this->assertEquals($expected, $this->router->action);
    }

      /**
     * @covers determineAction
     */
    public function testDetermineActionChangesControllerOnActionNotExist(){
        $this->router->getUrlSegments('/test/doesNotExist')
            ->determineController()
            ->createController()
            ->determineAction();

        $this->assertEquals($this->router->notFoundRoute[0], $this->router->controllerName);
        $this->assertEquals($this->router->notFoundRoute[1], $this->router->action);
        $this->assertEquals(\ErrorPages::class, $this->router->controller::class);
    }

    public function parameterDataProvider(){
        return [
            ['/test', []],
            ['/test/index', []],
            ['/test/index/one', ['one']],
            ['/test/index/one/two/three', ['one', 'two', 'three']],
            ['/test/index/3,d,/!#$/', ['3,d,', '!#$', '']],
            ['/text/index/index.html', ['index.html']]
        ];
    }

    /**
     * @covers determineParameters
     * @dataProvider parameterDataProvider
     */
    public function testDetermineParameters($url, $expected){
        $this->router->getUrlSegments($url)
            ->determineParameters();

        $this->assertEquals($expected, $this->router->parameters);
    }

    public function viewDataProvider(){
        return [
            ['/test/validView', 'ValidView'],
            ['/test/invalidView', '404']
        ];
    }

    /**
     * @covers determineView
     * @dataProvider viewDataProvider
     */
    public function testDetermineView($url, $expected){
        $this->router->execute($url);

        $this->assertEquals($expected, $this->router->view);
    }

    public function printedReturnDataProvider(){
        return [
            ['/test/printedReturnView', null],
            ['/test/printRReturnView', null]
        ];
    }

    /**
     * @covers determineView
     * @dataProvider printedReturnDataProvider
     */
    public function testDetermineViewWithPrintedReturnData($url, $expected){
        $this->router->execute($url);

        $this->expectOutputString('{"randomData":"dsdsddd"}');
        $this->assertEquals($expected, $this->router->view);
    }

    /**
     * @covers determineView
     */
    public function testDetermineViewChangesControllerOnViewNotExist(){
        $this->router->execute('/test/invalidView');

        $this->assertEquals('ErrorPages', $this->router->controllerName);
    }
}

I realize that there is probably a lot of room to grow here and I'm curious to know how I did this time around and where improvements can be made.

Almost forgot to add what the index.php file looks like now.

use xxxx\xxxx\Util\StorageUtils;
use xxxx\xxxx\Router\Router;

if(isset($argv)){
    $options = getopt('', ['uri::']);

    $url = $options['uri'];
}else{
    if($engine == 'nginx'){
        $url = $_SERVER['REQUEST_URI'];
    }else if($engine == 'apache'){
        $url = $_SERVER['PATH_INFO'];
    }
}

$router = new Router();
$router->setDefaultRoute('Home', 'index');
$router->setNotFoundRoute('ErrorPages', 'notFound');

$router->execute($url);
$router->executeView();
\$\endgroup\$
2
  • 1
    \$\begingroup\$ How are you going to inject dependencies to your controllers? new $controllerName() seems to imply that all your controllers must have parameterless constructors, which disallows to use constructor injection... \$\endgroup\$
    – slepic
    Dec 23, 2022 at 7:20
  • \$\begingroup\$ I can't think of a reason that a controller would need that capability at the moment @slepic \$\endgroup\$ Dec 24, 2022 at 1:59

2 Answers 2

4
\$\begingroup\$

Your router is doing too much.

Router should do one thing and one thing only. It should map request uri to a specific request handler (controller action).

You may get a better architecture if you think of router as being stateless. Actually this principle will serve you well far beyond just router.

Router should not instantiate controllers. That's job of controller factory. Furthermore controllers usually need some services to act upon. These services should be injected to the controller via constructor injection (there are other means of injection, but constructor injection is by far the best). Even if you don't see the need now, believe me it will show itself later. Unless you're doing something like calling everything via static methods, which is kinda bad on its own.

Router should not render views. That's job of your rendering engine to execute it. And it's job of controllers to invoke the execution.

Router also should not try to be the entire application. There usually is yet another class that uses a router to determine controller, but it is the application which then asks controller factory to instantiate controller and then it executes it but this happens after the router's job is already done.

Another way to think of router is just that it is a request handler itself. It is the controller for all routes and if receives a specific route for which there is a specific handler/controller, it delegates to it.

When I say "request handler" I mean specifically the RequestHandlerInterface of PSR-15 https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-15-request-handlers.md#21-psrhttpserverrequesthandlerinterface.

Consider implementing router such that it implements this interface and such that the handle method is the only public method of the router. To achieve configurabiloty of the router, you should split the implementation into two objects - router builder and the router itself. The builder has methods for configuration and creating of the router. Router is the RequestHandlerInterface implementation and all configuration is passed to it via constructor (inside the RouterBuilder::build() method).

$builder = new RouterBuilder($controllerFactory);
$builder->get('/test', 'Test', 'index');
$router = $builder->build();
$request = ServerRequest::fromGlobals();
$response = $router->handle($request);
sendResponse($response);
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for this advice. I will do more research on this and improve what I have done with your advice \$\endgroup\$ Dec 27, 2022 at 17:35
3
\$\begingroup\$

Code styling (refer to PSR-12 guidelines)

Avoid making loose comparisons whenever appropriate. != null will return true for a number of falsey values. If you want "not null", then use === null. If you are happy to greedily/loosely evaluate falsey values the simply use !$var.

Unless you have a good, sound reason to the contrary, avoid making single-use variable declarations like $url = explode.... Instead write $this->urlSegments = explode... in a single line.

if(count($this->urlSegments) > 0) can lose the > 0 and maintain the same logic because an array cannot have an negative count and an empty array will evaluate as falsey.

It doesn't make sense to check if(empty($controller)){ if the variable is guaranteed to be declared. Instead, you should should check if the first segment is a zero length string before converting its first character to uppercase (you might potentially want to look into title_casing which would make the whole word lowercase and the first letter uppercase).

if(count($this->urlSegments) > 1){ if(strlen($this->urlSegments[1]) > 0){ seems reasonably condensed into one statement with &&. It also feels reasonable to just check !empty($this->urlSegments[1]) since the action can be presumed to be a "word".

Overall, I see a lot of sensible method naming and the single-responsibility principle looking fairly implemented. While it is possibile that I've overlooked something in the "big picture", I'd say this latest code isn't: "wow this is bad".

\$\endgroup\$
3
  • \$\begingroup\$ Thank you for these suggestions I can definitely see the improvement in code quality. Having the test to prove the code still worked after every change has been a real game changer and honestly I have no clue how I ever programmed before \$\endgroup\$ Dec 22, 2022 at 22:22
  • \$\begingroup\$ I feel as if I was tooting my own horn with that comment... I mean that to say implementing your suggested changes made the code better quality thank you \$\endgroup\$ Dec 22, 2022 at 23:22
  • \$\begingroup\$ @JoséphFlames I recommend that you seriously consider the advice that slepic has offered (and I will not be insulted if you move the green tick). My answer is a very low-level critique. Their answer is much higher-level and therefore more likely to put you onto better coding practices for the given task. I have not spent much time (in my career) considering the best design pattern(s) for writing routers. \$\endgroup\$ Dec 25, 2022 at 3:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.