[CODE REVIEW] Simplu router php

Salut, incerc sa creez un simplu router in php. Momentan inca sant la inceput cu el. Ce as putea imbunatati dupa parerea voastra? E ceva gresit?
Atasez si clasele.
Clasa request se ocupa de requestul http

final class Request{
const HTTP_VERBS = 'GET|POST|PUT|DELETE';
final public function isAjax(){
	 return isset($_SERVER['HTTP_X_REQUESTED_WITH']) && strtolower($_SERVER['HTTP_X_REQUESTED_WITH']) === 'xmlhttprequest' ? true : false;
}
final public function rawUri(){
	 return $_SERVER['REQUEST_URI'];
}
final public function cleanUri(){
	 return preg_replace('/[^\da-z\-\/]/i', '', filter_var($_SERVER['REQUEST_URI'], FILTER_SANITIZE_URL));
}
final public function getRequestMethod(){
	 $method = isset($_SERVER['REQUEST_METHOD']) && !empty($_SERVER['REQUEST_METHOD'])
&& preg_match('/'.self::HTTP_VERBS.'/', $_SERVER['REQUEST_METHOD']) ? strtoupper($_SERVER['REQUEST_METHOD']) : 'GET';
	 if($method == 'POST' && isset($_POST['_method']) && !empty($_POST['_method'])){
		 if($method = $_POST['_method'] == 'PUT' || $method == 'DELETE')
			 return $method;
		 throw new HttpRequestMethodException("Invalid Http request method.");
	 }
	 return $method;
}
public function getIp(){
	 if(function_exists('apache_request_headers'))
		 $headers = apache_request_headers();
	 else
		 $headers = $_SERVER;
	 if(array_key_exists('X-Forwarded-For', $headers) && filter_var($headers['X-Forwarded-For'], FILTER_VALIDATE_IP))
		 $the_ip = $headers['X-Forwarded-For'];
	 elseif(array_key_exists('HTTP_X_FORWARDED_FOR', $headers) && filter_var($headers['HTTP_X_FORWARDED_FOR'], FILTER_VALIDATE_IP))
		 $the_ip = $headers['HTTP_X_FORWARDED_FOR'];
	 else
		 $the_ip = filter_var($_SERVER['REMOTE_ADDR'], FILTER_VALIDATE_IP);
	 return $the_ip;
}
}

O simpla clasa pt a face o colectie de rute. Implementez IteratorAgregate si Countable

final class RouteCollection implements IteratorAggregate, Countable{
private $collection = [];
public function __construct(){}
public function getIterator(){
	 return new ArrayIterator($this->collection);
}
public function count(){
	 return sizeof($this->collection);
}
public function addElement(Route $route){
	 $this->collection[] = $route;
}
}

Fiecare ruta e o instanta a clasei Route

final class Route{
private $callback, $regex, $args = false;
private $patterns = [
	 ':string' => '([a-z]+)',
	 ':numeric' => '(\d+)',
	 ':alfanumeric' => '(\w+)'
];
public function __construct($callback, $regex){
	 $this->callback($callback);
	 $this->compileRegex($regex);
}
private function compileRegex($regex){
	 $regex = "/^".str_replace("/", "\/", $regex)."$/i";
	 $this->regex = str_replace(array_keys($this->patterns), array_values($this->patterns), $regex);
}
private function callback($callback){
	 if(strpos($callback, '@') !== false){
		 $this->callback = explode('@', $callback);
	 }else{
		 $this->callback = $callback;
	 }
}
public function match($input){
	 if(preg_match($this->regex, $input, $out)){
		 $this->args = $out;
		 return true;
	 }
	 return false;
}
public function getCallback(){
	 return $this->callback;
}
public function getArgs(){
	 return $this->args;
}
}

Si router

final class Router{
private static $instance = null;
private $get, $post, $put, $delete, $request;
public static function init(){
	 is_null(self::$instance) ? self::$instance = new self() : false;
	 return self::$instance;
}
private function __construct(){
	 $this->get	 = new RouteCollection();
	 $this->post = new RouteCollection();
	 $this->put	 = new RouteCollection();
	 $this->delete = new RouteCollection();
	 $this->request = new Request();
}
public function addRoute($method, $callback, $regex){
	 $this->{strtolower($method)}->addElement(new Route($callback, $regex));
}
public function dispatch(){
	 $verb = strtolower($this->request->getRequestMethod());
	 foreach($this->{$verb} as $key => $value){
		 if($value->match($this->request->cleanUri())){
			 if($value->getArgs() > 1)
				 call_user_func_array($value->getCallback(), $value->getArgs());
			 else
				 call_user_func($value->getCallback());
			 return true;
		 }
	 }
	 throw new InvalidArgumentException('404');
}
}

Si cod prin care exemplific functionalitatea:
index.php

define("BASE_PATH" , realpath(__DIR__).DIRECTORY_SEPARATOR);
define("APP_PATH" , BASE_PATH."app".DIRECTORY_SEPARATOR);
define("TEMP_PATH" , BASE_PATH."temp".DIRECTORY_SEPARATOR);
define("SYSTEM_PATH", BASE_PATH."system".DIRECTORY_SEPARATOR);
define("UI_PATH" , BASE_PATH."ui".DIRECTORY_SEPARATOR);
try{
require_once SYSTEM_PATH."ClassLoader.php";
ClassLoader::init([APP_PATH, SYSTEM_PATH]);
$route = Router::init();
$route->addRoute('get', 'index1@start', '/sites/acasa/test/:numeric');
$route->addRoute('get', 'index1@alta', '/sites//acasa');
$route->addRoute('get', 'index1@de_forma', '/sites/acasa/:string');
$route->addRoute('get', 'index1@complex', '/sites/:string/test/:numeric');
$route->addRoute('get', 'test', '/sites/test');
$route->dispatch();
}catch (Exception $e){
print_r($e->getTrace());
die($e->getMessage());
}
function test(){
echo 'test din functie';
}

Clasa de test index1

class index1{
function start(){
	 $params = func_get_args();
	 echo "metoda start".$params[1];
}
function alta(){
	 echo 'alta342423';
}
function de_forma(){
	 $params = func_get_args();
	 echo "metoda de_forma ".$params[1];
}
function complex(){
	 $params = func_get_args();
	 echo "metoda complex ".$params[1]."--".$params[2];
}
function index(){
	 echo 'default';
}
}

Folosesc rewrite engine din apache

Options +FollowSymlinks -MultiViews -Indexes
RewriteEngine On
RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteCond %{REQUEST_URI} !^.*\.(css|js|jpe?g|gif|png|ico)$ [NC]
RewriteRule ^(.*)$ index.php/$1

Pt moment va rog ignorati throw new InvalidArgumentException('404'); din Router->dispatch() este doar pt teste.De incluziunea claselor se ocupa ClassLoader un simplu autoloader.

Edited by danutz0501, 30 January 2015 - 18:24.

• Prea mult static si prea mult final
  
• pe alocuri codul e haotic. Vezi de exemplu apelurile in locuri diferite ale lui filter_var la treaba cu IP-ul.
  
• foloseste PSR-4, vezi de exemplu composer, sau scrie-ti propriul autoloader conform standardului (eu nu as face-o, in ecosistemul PHP5.3+, varianta cu composer e cea curata)
  
• lipsesc testele, lipseste testabilitatea
  
• Indentare haotica, greu de urmarit. Mai bine publica codul intr-un proiect pe github ceva, pe acest forum formatarea lasa de dorit.
  
• prea multe accese la superglobale. Paseaza-le constructorilor - vezi punctul anterior cu testabilitatea, go figure.

Edited by OriginalCopy, 30 January 2015 - 18:45.

Ms de raspunsuri. O sa curat un pic codul, adaug comentarii si o sa postez un link catre un repo pe github

Am adaugat mici comentarii si linkul https://github.com/d...lDumitru/router
PS: trebuie sa ma apuc sa invat TDD sant zero la capitolul unit testing

Edited by danutz0501, 31 January 2015 - 01:34.