0
\$\begingroup\$

Can you review my code?

class Router {

    /**
     * Store the current URI [0] => Controller/ [1] => Method/ [2]=> parameter
     * @var array
     */
    private $URI = array();

    /**
     * Store an instance of configuration object
     * @var object 
     */
    private $config = NULL;

    /**
     *  Store an instance of Application object
     * @var object 
     */
    private $app = NULL;

    /**
     * Initialize the router
     */
    public function __construct() {
        $this->startup();
    }

    /**
     * 
     * @param object $route
     * @return string
     */
    public function get($route) {
        return $this->config->getRoute($route);
    }

    /**
     * First step, to catch the url and manipulate it.
     */
    private function startup() {

        if ($url = $this->getURL()) {

            $this->app = Application::getAppInstance();
            $this->config = Application::getInstance('Configuration');         

            if ($this->validateURL()) {
                $class = (isset($url[1])) ? $url[1] : $this->get('defaultController');
                unset($url[1]);
                $method = (isset($url[2])) ? $url[2] : $this->get('defaultMethod');
                unset($url[2]);
                $params = (count($url > 0)) ? $url : "";
                unset($url);

                if (class_exists($class)) {
                    $controller = $this->app->getInstance($class);
                } else {
                    exit("Class not found");
                }
                if (method_exists($controller, $method)) {
                    if ($params !== "") {
                        call_user_func_array(array($class, $method), $params);
                    } else {
                        $controller->{$method}();
                    }
                } else {
                    exit("Method not found");
                }
            } else {
                exit("URI chars not allowed");
            }
        } else {
            if ($this->get('defaultController') !== "") {
                $this->app->getInstance($this->get('defaultController'));
                call_user_func(array($this->get('defaultController'), $this->get('defaultMethod')));
            } else {
                echo "Default does not added.";
            }
        }
    }

    /**
     * Get the current url and convert into an array 
     * @return array
     */
    private function getURL() {
        $currentURL = $_SERVER['QUERY_STRING'];
        if ($currentURL !== "") {
            $currentURL = rtrim($currentURL, '/');
            $currentURL = explode('/', $currentURL);
            unset($currentURL[0]);
            $this->URI = $currentURL;
            return $currentURL;
        }
        return false;
    }

    /**
     * Validate the URI characters
     * @return boolean
     */
    private function validateURL() {
        foreach ($this->URI as $uri) {
            if (!preg_match("/^[" . $this->config->get('allow_uri_chars') . "]+$/i", $uri)) {
                return false;
            }
        }
        return true;
    }

}
\$\endgroup\$
2
\$\begingroup\$
$params = (count($url > 0)) ? $url : "";

Is this really what you meant to say? This looks like it should be

 $params = (count($url) > 0) ? $url : '';

The second version treats the $url variable as an array. The previous code treats $url as an integer and then treats a Boolean expression as an array. This is problematic because both count(false) and count(true) return 1. Meaning that the first version is the same as $params = $url.

This was the only functional comment that I had on the code. Everything else is just style. It's possible that if you fleshed out a problem statement describing what the code is supposed to do that someone would have additional functional comments.

            if ($this->validateURL()) {
                $class = (isset($url[1])) ? $url[1] : $this->get('defaultController');
                unset($url[1]);
                $method = (isset($url[2])) ? $url[2] : $this->get('defaultMethod');
                unset($url[2]);
                $params = (count($url > 0)) ? $url : "";
                unset($url);

                if (class_exists($class)) {
                    $controller = $this->app->getInstance($class);
                } else {
                    exit("Class not found");
                }
                if (method_exists($controller, $method)) {
                    if ($params !== "") {
                        call_user_func_array(array($class, $method), $params);
                    } else {
                        $controller->{$method}();
                    }
                } else {
                    exit("Method not found");
                }
            } else {
                exit("URI chars not allowed");
            }

You could reduce your indentation by using early exits instead of if/else.

            if (!$this->validateURL()) {
                exit('URI chars not allowed');
            }

            switch (count($url)) {
                default:
                    $method = (isset($url[2])) ? $url[2] : $this->get('defaultMethod');
                    unset($url[2]);
                    // 3 or more fall through to 2
                case 2:
                    $class = (isset($url[1])) ? $url[1] : $this->get('defaultController');
                    unset($url[1]);
                    // 2 or more fall through to 1
                case 1:
                    $params = $url;
                    break;
                case 0:
                    $params = '';
                    break;
            }
            unset($url);

            if (!class_exists($class)) {
                exit('Class not found');
            }

            $controller = $this->app->getInstance($class);
            if (!method_exists($controller, $method)) {
                exit('Method not found');
            }

            if ('' === $params) {
                $controller->{$method}();
            } else {
                call_user_func_array(array($class, $method), $params);
            }

I also changed the order of the final if/else. I try to avoid the if not else form. An else is implicitly taking the logical not of the test expression. So if the test expression is negative, this means that the else is the negative of a negative. I often find it more readable if I reorder the clauses such that the test expression is positive.

I prefer to use single quotes if I can. There's a slight performance benefit, but the chief advantage is that it is easy to see that no variable interpolation will occur. With double quotes you have to examine the string to tell that.

I don't know that the switch version is better than what you had originally, but I wanted to include it as a possibility. I find it a little clearer, but it is more code. On average, it may give a very slight performance advantage. Probably not enough to matter.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your review. concerning $params = (count($url > 0)) ? $url : ""; I have used to choose between whether use call_user_func_array(array($class, $method), $params); or $controller->{$method}();. also you have a right concerning if/else I have have been dispensed with else. but I don't know I don't accept to use switch statement. \$\endgroup\$ – Lion King Jul 29 '15 at 0:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.