current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite
1

I just finished rewriting my Router class. Any input would be much appreciated.

The class routes URLs based on a site.com/class/method/param/param/etc syntax.

One thing I've done differently is created a mechanism for basic templates to be loaded directly without the need for a controller. I often find pages such as /about/ do not require additional methodology, and setting up really baisc controllers just seems like a waste of time. Therefore, I have created a check for templates such as "_about.php" in the views folder, with the underscore denoting that it can be loaded directly.

The class also currently assumes an error class or template.

class Router{

    public static function getPath($uri = null){
        // set default uri
        $uri = is_null($uri) ? $_SERVER['REQUEST_URI'] : $uri;

        // remove possible query string
        $uri = preg_replace('/\?(.*)/', '', $uri);

        return explode('/',trim($uri,'/'));
    }

    public static function getQuery($uri = null){

        // set default uri
        $uri = is_null($uri) ? $_SERVER['REQUEST_URI'] : $uri;

        // get query string
        $query = substr($uri, strpos($uri,'?') + 1);

        // parse into array
        $q = array();
        parse_str($query,$q);

        return $q;
    }

    public static function follow($uri = null){

        $path = self::getPath($uri);

        // get controller and method, or set defaults of root and index
        $controller = ($path[0] == "") ? 'root' : $path[0];
        $method = ((count($path) > 1) && ($path[1] != "")) ? $path[1] : null;

        // see if controller is a static page, only possible if no method specified
        if(is_null($method) && file_exists(APPLICATION_DIR . '/views/_' . $controller . '.php')){
            include APPLICATION_DIR . '/views/_' . $controller . '.php';
        }
        else if(file_exists(APPLICATION_DIR . '/controllers/' . $controller . '.php')){ // see if controller exists
            // load controller class
            include_once APPLICATION_DIR . '/controllers/' . $controller . '.php';

            // if method is empty or doesn't exist, it defaults to "index"

            if(is_null($method) || !method_exists($controller,$method)) {
                $method = "index";
            }

            if(method_exists($controller, $method)){
                $c = new $controller();
                $c->$method();
            }
            else{
                self::follow('e404');
            }
        }
        else{
            self::follow('e404');
        }

    }

}
share|improve this question

2 Answers 2

up vote 4 down vote

This is not OOP.

Observe how your class contains only static functions. There are no class properties. This can be written equivalently:

namespace Router;

function getPath($uri = null){
    // Same implementation (I have just cut it out for readability).
}

function getQuery($uri = null){
    // Same implementation.
}

function follow($uri = null){
    // Same implementation.
}

Usage from anywhere can be compared (First your code):

Router::getPath($uri);
Router::getQuery($uri);
Router::follow($uri);

Then the equivalent:

Router\getPath($uri);
Router\getQuery($uri);
Router\follow($uri);

Note how no object is required to be used (that is a good hint that its not OO).

The following question would be worth reading:

Is this good class design (OOP PHP)?

I have an answer to that question which covers in more detail some of the problems with using static in OOP.

share|improve this answer
    
You are correct, I was not paying attention. Actually, to be perfectly honest, I just didn't know, I'm quite new to OOP myself, so I'll be taking a look at that answer myself :) +1 –  mseancole Apr 29 '12 at 13:57
    
Thanks for the input. My intention with the title was that the router was for an OOP architecture, loading classes and methods. I probably should have been clearer. I haven't really used namespaces before but understand the concept. I'll look into it more. Thanks –  christian Apr 29 '12 at 14:46
    
MVC is an architecture that can be implemented using Object Oriented, Procedural, Functional and I'm guessing Logical paradigms. Splitting your way of thinking at this point of the problem does not make sense to me. Why not have an OO router in an OO system? –  Paul May 1 '12 at 13:28
    
I can see your point, but then I'd be required to instantiate an object every time I wanted to use the router. I can see where benefits could come in if I ever want more complex methods, but at the present time I'm looking at a one step process instead of a two. At least until PHP 5.4 is more readily available. –  christian May 3 '12 at 1:28
up vote 1 down vote

This is really good, there are only a few things I would point out.

Why are you using is_null()? All this does is check for a null value. It would be equivalent to doing ! $uri, except that would have the added benefit of checking for FALSE. Check out filter_var() if your PHP version is >= 5.2. I find myself suggesting this function all the time, I should macro it or something...

$uri = filter_var($uri, FILTER_VALIDATE_URL) ?: $_SERVER['REQUEST_URI'];

You could also use the FILTER_SANITIZE_URL flag. I did not add it myself, because I am horrible with regex, and have no idea what you are doing there. If you wish to add it, separate it from the other flag with a bitwise "Or" operator |.

Instead of retyping those long file names, you should make them variables. It saves time and effort, and removes one more place for an error to occur. So...

    $controller = ($path[0] == "") ? 'root' : $path[0];
    $method = ((count($path) > 1) && ($path[1] != "")) ? $path[1] : null;

    $view_file = APPLICATION_DIR . '/view/_' . $controller . '.php';
    $controller_file = APPLICATION_DIR . '/controllers/' . $controller . '.php';

You should not depend on your calling application to provide functionality. APPLICATION_DIR came from no where, you should declare it locally, or at least provide some check to ensure it is set. Failing this, you should at the very least, comment your code to tell where that constant is supposed to come from.

Why did you default $method to null if you are just going to default it to 'index' later? You haven't used it for anything before you changed it to index, so I find it rather confusing... When you initially set it, just set the default to index.

Also, what happens if 'e404' somehow gets deleted or moved? You will be stuck in a continuous loop. I would just manually set a 404 page or somehow ensure that this code does not get stuck.

Good Luck!

share|improve this answer
    
Thanks! The router is actually part of a small framework I am developing. I will make sure to make notes on constants. Good call on the "index" default value and the use of is_null. My first thought with is_null was to allow for "0" values, but that doesn't really make a difference in this usage. I also agree with the e404 issue. The possible infinite loop was one of my main concerns. However, I like the ability to have either a class or a simple template, so I will probably just add an additional check at the end to see if the controller is already e404 and if so output a simple die message. –  christian Apr 29 '12 at 6:33

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.