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. 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);
    }
}
share|improve this question
2  
Welcome to Code Review, as per the FAQ you should post specific pieces of code to be reviewed (a class or method for example) can you update your question? –  Trevor Pilley Oct 31 '12 at 11:13
    
Just to clarify, a link to code is not sufficient. The question needs to live long after a pastebin has disappeared for it to be appropriate for this site. –  Paul Oct 31 '12 at 23:59
    
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
add comment

1 Answer 1

up vote 0 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
    
i'll come back with a new version of the file :-) ( been working on it on my free time ) now it has been split into two separate files a class that handles only dispatching and a matcher.. :-) –  Bogdan Dec 17 '12 at 22:27
add comment

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.