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 have finished my router and would like your thoughts on anything that may be inefficient of could be done better!:

class Router {

public $start_page = 'Dashboard' ; // Change to your home/default page.
public $staticRoutes = array(
    'alias' => 'ActualController',
) ;

function __construct()
{
    $url = substr(rtrim($_SERVER['REQUEST_URI'], '/'), 6) ;
    if($url)
    {
        $components = explode('/', $url, 3) ;
        if(array_key_exists($class, $this->staticRoutes))
        {
            $class = $this->staticRoutes[$class] ;
        } else {
            $class = preg_replace('/[^a-z-]/', '', $components[0]) ;
        }

        if(class_exists($class))
        {
            if(isset($components[1])) // has method...
            {
                $method = preg_replace('/[^a-z-]/', '', $components[1]) ;
                if(method_exists($class, $method))
                {
                    if(isset($components[2])) // has params...
                    {
                        $params = explode('/', $components[2]) ;
                        call_user_func_array(array($class, $method), $params) ;
                    } else {
                        $controller = new $class ;
                        $controller->$method() ;
                    }
                } else {
                    $e = 'Method '.$components[1].' does not exist.' ;
                    new Error(array('msg' => $e), 'Method Error') ;
                }
            } else { // No method so just go to default index if exists...
                $controller = new $class ;
                if(method_exists($controller, 'index'))
                {
                    $controller->index() ;
                } else {
                    $e = 'Class '.$class.' has no default index method.' ;
                    new Error(array('msg' => $e), 'Class Index Error') ;
                }
            }
        } else {
            $e = 'Class '.$components[0].' does not exist.' ;
            new Error(array('msg' => $e), 'Class Error') ;
        }
    } else {
        $controller = new $this->start_page ;
        $controller->index() ;
    }       
}

}

Note that I have not implements the static routes functionality yet.

share|improve this question
    
It has access to global state (in $_SERVER), which is a bad idea. Wrap all data from the request into a request object and pass it to the router. There you can examine it without accessing global state. You can also test your router in a unit test, which does not know about any real HTTP requests. And by the way: Your Router is actually more like a FrontController. –  Sven Dec 5 '12 at 22:27
    
What about throwing an exception if an error occurs? –  Sven Dec 5 '12 at 22:28
    
@Sven do you know of any examples of a request object? What do you mean throwing an exception? If an error occurs the user is sent to an error page (class Error). –  imperium2335 Dec 7 '12 at 19:01

1 Answer 1

Your constructor is doing too much. Just because the only thing you are going to do with your router is initialize it, that doesn't mean you can't have more methods. And as Sven pointed out, it probably shouldn't have access to the global state. Inject whatever you need.

You have two public properties, one uses underscores the other camelCase. I would suggest picking a particular naming scheme and sticking to it. Consistency is important.

Your code seriously violates the arrow anti-pattern. You might consider refactoring to remove some of the indentation and if/else statements. For instance, reversing the first if/else and returning early will allow you to then drop the else, thus removing one level of indentation from your entire method.

if( ! $url ) {
    $controller = new $this->start_page;
    $controller->index();
    return;
}

//etc...

I've never been one for magic numbers and these vague array keys bug me. It shouldn't be necessary to provide comments to understand what each array key does. I would consider padding an array to ensure the size then creating a list. At the same time you can run each component through a loop to clean it. I'm wanting to say you can use array_map() or array_walk() instead of the loop, but I don't feel like trying to figure that out right now. I'm not quite sure what your REGEX is doing so I don't know if there is a better way, but check out filter_var(), that may do what you need.

$components = explode( '/', $url, 3 );
foreach( $components as $key => $component ) {
    $components[ $key ] = preg_replace( '/[^a-z-]/', '', $component );
}
$components = array_pad( $components, 3, FALSE );

list( $class, $method, $params ) = $components;

By the way, where did $class originally come from? The first time I see it is when you check to see if it exists in the $staticRoutes property. If that is a global you should know that globals are evil and you should be ashamed.

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.