Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
        }
    }
share|improve this question
    
Am i right that you are mapping one route to one specific controller? –  Peter Kiss Nov 1 '12 at 17:21
    
yes i'm mapping one route to one specific controller –  Bogdan Nov 3 '12 at 13:54

1 Answer 1

up vote 1 down vote accepted

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

}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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