Review: Finished router class for custom mini framework

Question

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.

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.