Routing done in php

Question

I've written a small routing system in PHP but I'm not sure if it's done the right way and what changes can they be done to the code and improvements. Thank you very much for your help and time to read this post.

namespace Twelve\Router;
class Router
{
    public
        $routeMaps = [],
        $controller = [],
        $params = [];
    public function add($routeName, $routePattern, $routeController, $routeParams = [])
    {
        $this->routeMaps[$routeName] =
        [
            'pattern' => $routePattern,
            'routeController' => $routeController,
            'params' => $routeParams,

        ];
    }
    public function checkMatch($url)
    {
        foreach ($this->routeMaps as $routeMap) {
            // Find {params in URI}
            if (preg_match_all('/\{(?:[^\}]+)\}/', $routeMap['pattern'], $this->matches, PREG_SET_ORDER)) {
                foreach ($this->matches as $this->isMatch) {
                    // Swap {param} with a placeholder
                    $this->uri = str_replace($this->isMatch, "%s", $routeMap['pattern']);
                }

                // Build final Regex
                $this->finalRegex = '/^' . preg_quote($this->uri, '/') . '$/';
                $this->finalRegex = vsprintf($this->finalRegex, $routeMap['params']);
                // Let's test the route.
                if (preg_match($this->finalRegex, $url)) {
                    $this->setController($routeMap['routeController']);
                    $this->setAction($this->getController());
                    $this->setParams(array_slice(explode('/', $url), '4'));

                    return true;
                } else

                    return false;
            }
        }
    }

    public function dispatch()
    {
        array_pop($this->controller);
        $this->controller = implode('\\', $this->controller);
        call_user_func_array([new $this->controller, $this->action],  $this->getParams());
    }
    public function setController($controller)
    {
        $this->controller = explode(':', $controller);
    }
    public function getController()
    {
        return $this->controller;
    }
    public function setAction($action)
    {
        $this->action = end($action);
    }
    public function getAction()
    {
        return $this->action;
    }
    public function setParams($params)
    {
        $this->params = $params;
    }
    public function getParams()
    {
        return $this->params;
    }
    public function run($uri)
    {
        if($this->checkMatch($uri))
            $this->dispatch();
        else
            echo "Is not a valid Resource";
    }
}

This is a new version of the file

use Twelve\Utils\Utils;
class Router
{
    public
        $routeMaps = [],
        $controller;
    private $_route = [];

    public function add($name, $pattern, $controller, array $params = [])
    {
        if(!isset($this->routeMaps[$name]))
            $this->routeMaps[$name] =
        [
            'pattern' => $pattern,
            'controller' => $controller,
            'params' => $params,
        ];
    }
    public function findMatch($url)
    {
        foreach($this->routeMaps as $routeMap)
        {
            $this->regex = $this->buildRegex($routeMap['pattern'], $routeMap['params']);
            // Let's test the route.
            if(preg_match($this->regex, $url))
            {
                return (array) $routeMap['controller'];
            }
        }
    }
    public function buildRegex($uri, array $params)
    {
        // FInd {params} in URI
        if(preg_match_all('/\{(?:[^\}]+)\}/', $uri, $this->matches, PREG_SET_ORDER))
        {
            foreach($this->matches as $isMatch)
            {
                // Swap {param} with a placeholder
                $this->uri = str_replace($isMatch, "%s", $uri);
            }
            // Build final Regex
            $this->finalRegex = '/^' . preg_quote($this->uri, '/') . '$/';
            $this->finalRegex = vsprintf($this->finalRegex, $params);

            return $this->finalRegex;
        }
    }
    public function dispatch(array $route = [], $url)
    {
        $this->setController($route['0']);
        $this->setAction($this->getController());
        $this->setParams(explode('/', $url));
        $this->controller = $this->getController();
        array_pop($this->controller);
        $this->controller = implode('\\', $this->controller);
        call_user_func_array([new $this->controller, $this->action], $this->params);
    }

    public function setController($controller)
    {
        $this->controller = explode(':', $controller);
    }

    public function getController()
    {
        return $this->controller;
    }
    public function setAction($action)
    {
        $this->action = end($action);
    }
    public function getAction()
    {
        return $this->action;
    }

    public function setParams($params)
    {
        $this->params = array_slice($params, 4);
    }

    public function getParams()
    {
        return $this->params;
    }
    public function run($uri, array $route = null)
    {
        $route = $this->findMatch($uri);
        Utils::var_dump($route);
        $this->dispatch($route, $uri);
    }
}

Answer 1

Here are my observations

NOTE I did all this via stream-of-conscientiousness and I haven't tested any of these changes (or your original code) so take these as guidance and not exact changes :)

router.php

<?php
class Router
{
    // TODO You need to use `private` a lot more for encapsulation
    private $routeMaps = [];

    // TODO This function needs to document the expected format of inputs
    // TODO is initial controller format supposed to be 'full:class:path:method' ?
    public function add($name, $pattern, $controller, array $params = [])
    {
        // TODO What if it is already set?
        if(!isset($this->routeMaps[$name]))
        {
            $this->routeMaps[$name] =
            [
            'pattern'    => $pattern,
            'controller' => $controller,
            'params'     => $params,
            // Build the regex now so we ensure we only build it once
            'regex'      => $this->buildRegex($pattern, $params),
            ];
        }
        else
        {
            throw new Exception("Route '{$name}' already defined");
        }
    }

    // TODO Document function

# why the '$route = null'?  It is not used
#?? public function run($uri, array $route = null)
    public function run($uri)
    {
        $routeMap = $this->findMatch($uri);
#       Utils::var_dump($routeMap);
        // TODO check to see if we actually found a match
        if (null !== $routeMap)
        {
            // TODO we should probably parse the parms before calling dispatch
            $this->dispatch($routeMap, $uri);
        }
        // TODO probably want to notify user somehow
        else
        {
            ; // ??
        }
    }

    // TODO Why are you calling this parm $uri instead of $pattern?
    private function buildRegex($uri, array $params)
    {
        // FInd {params} in URI
        if(preg_match_all('/\{(?:[^\}]+)\}/', $uri, $matches, PREG_SET_ORDER))
        {
            foreach($matches as $isMatch)
            {
                // Swap {param} with a placeholder
# matching $uri and setting $this->uri means only the last match is saved
# Is that what you want?  If so, document it
#??             $this->uri = str_replace($isMatch, "%s", $uri);
                $uri = str_replace($isMatch, "%s", $uri);
            }
            // Build final Regex
            $finalRegex = '/^' . preg_quote($this->uri, '/') . '$/';
            $finalRegex = vsprintf($finalRegex, $params);

            return $finalRegex;
        }
        // TODO Need to throw error if uri doesn't match
        else
        {
            throw new Exception("Invalid uri: '{$uri}'");
        }

    }

    // TODO Why does (did) this return an array of one element, containing just the
    //      controller?
    // TODO Shouldn't this return the routemap instead of just the controller?
    private function findMatch($url)
    {
        foreach($this->routeMaps as $routeMap)
        {
            // Let's test the route.
            if(preg_match($routeMap['regex'], $url))
            {
                return $routeMap;
            }
        }

        return null; // TODO We should return something if no match
    }

    // TODO This should take the already parsed $parms    
    private function dispatch($routeMap, $url)
    {
        $parts = explode(':', $routeMap['controller']);
        $action = array_pop($parts);
        $clazz = implode('\\', $parts);
        $params = $this->parseParams($url);
        call_user_func_array([new $clazz, $action], $params);
    }

    // TODO This needs to be based on the pattern in the rout map
    // TODO Since it appears that the 'params' in the route map have to
    //      match in order for the url to match, we should propably use
    //      the length of those in our parsing here.
    // TODO Perhaps we could even use the regex to tell these values at
    //      match time?
    private function parseParams($url)
    {
        return  array_slice(explode('/', $url), 4);
    }

}