Small routing system

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.

<?php
namespace Twelve\Router;

    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);
    }

}