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.

Based off simple controller implementations like those seen in micro-frameworks like Slim, Silex, F3, Laravel, Tonic, Flight, Klein, etc.

/EDIT tried to link the above sources, but as a new user I only get 2 links...

Any potential issues? Improvements?

class Router {   

    private static $routes = array();

    private function __construct() {}
    private function __clone() {}

    public static function route($pattern, $callback) {
        $pattern = '/' . str_replace('/', '\/', $pattern) . '/';
        self::$routes[$pattern] = $callback;
    }

    public static function execute() {
        $url = $_SERVER['REQUEST_URI'];
        $base = str_replace('\\', '/', dirname($_SERVER['SCRIPT_NAME']));
        if(strpos($url, $base) === 0) {
            $url = substr($url, strlen($base));
        }
        foreach (self::$routes as $pattern => $callback) {
           if(preg_match($pattern, $url)){
                preg_match_all($pattern, $url, $matches);
                array_shift($matches);
                $params = array();
                foreach($matches as $match){
                    if(array_key_exists(0, $match)){
                        $params[] = $match[0];
                    }
                }
                return call_user_func_array($callback, $params);
           }
        }
    }   
}

Takes a regexp and a callback, checks the URI for the first match... e.g.,

Router::route('blog/(\w+)/(\d+)', function($category, $id){
  print $category . ':' . $id;
});
Router::execute();
// if url was http://example.com/blog/php/312 you'd get back "php:312"...

TYIA.

share|improve this question
add comment

1 Answer 1

up vote 1 down vote accepted

I'm not seeing why you preg_match_all, considering you (can!) really only deal with the first match. Why not something like

if (preg_match($pattern, $url, $params)) {
    array_shift($params);
    return call_user_func_array($callback, array_values($params));
}

The str_replace to build a regex feels rickety to me; if the pattern were, say, '\/', it'd get regexified wrong and give you a PHP warning. (PCRE is pretty consistent about saying that any punctuation, metacharacter or not, can be escaped without ill effects. Blindly prefixing every / with a \ breaks that.) I'm not sure how you'd fix that, but one way to sidestep the problem might be to require that $pattern already include the delimiters rather than the router trying to tack them on itself. Besides that, it'd also serve as a clear "this is a PCRE-flavored regex" indicator, so the rules are known and people are less likely to accidentally use stuff like . without escaping it.

I'm thinking that private function __clone() {} isn't strictly necessary, and in fact hints that an instance may somehow exist. (If there's no instance possible, there's nothing to clone, and thus no need to prevent cloning.)

share|improve this answer
    
very good point, i'll implement your suggestion. Anything else you see? Any potential problem areas? –  momo Nov 9 '12 at 2:54
    
@momo: Just updated with other things i considered. –  cHao Nov 9 '12 at 4:40
    
great feedback. i'd upvote but need more rep. thanks again. –  momo Nov 9 '12 at 16:10
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.