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

• Peter Kiss: Am i right that you are mapping one route to one specific controller?
• Bogdan: yes i'm mapping one route to one specific controller

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