current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

For the request object in my PHP application, I need to dissect the URL and assign its components to my class attributes (module, controller, method and the rest are parameters). Currently, I'm doing it like the following but that looks really ugly to me. Is there a better way to do it?

$urlComponents = explode('/', $requestUrl);

if(isset($urlComponents[0]))
{
    $this->_module = array_shift($urlComponents);

    if(isset($urlComponents[0]))
    {
        $this->_controller = array_shift($urlComponents);

        if(isset($urlComponents[0]))
        {
            $this->_method = array_shift($urlComponents);

            if(isset($urlComponents[0]))
            {
                $this->_params = $urlComponents;
            }
        }
    }
}
share|improve this question
    
Welcome to Code Review! Your title should indicate what the code does, the with to improve on the code is implied by posting it here. – Mast Oct 31 at 13:02

2 Answers 2

up vote 2 down vote accepted

There is really no need to use array_shift like you are doing. You can just access elements of an array like this: urlComponents[1], which is a lot easier to read. With this, your code would look like this:

$urlComponents = explode('/', $requestUrl);
if(isset($urlComponents[0])) {
    $this->_module = $urlComponents[0];
}
if(isset($urlComponents[1])) {
    $this->_controller = $urlComponents[1];
}
if(isset($urlComponents[2])) {
    $this->_method = $urlComponents[2];
}
$this->_params = array_slice($urlComponents, 3);

Although I would assume that a module, controller, and method must be given, so I would probably write it like this:

$urlComponents = explode('/', $requestUrl);

if (count($urlComponents) < 3) {
    throw new Exception('Cannot call method because of missing values for url: ' . $requestUrl);
}

$this->_module = $urlComponents[0];
$this->_controller = $urlComponents[1];
$this->_method = $urlComponents[2];
$this->_params = array_slice($urlComponents, 3);

Also note that you probably want to remove a trailing slash from the $requestUrl first to avoid getting empty values.

share|improve this answer
up vote 0 down vote

Here's an alternative implementation. This seems to be more readable to me, by using list() to define $module,$controller, and $method. In the end its a matter of personal style, but i think the following provides a logical approach to how you would ideally like to layout your code for matching a route in a MVC application.

$urlComponents = explode('/', $requestUrl);

if(count($urlComponents) > 3){

    list($module,$controller,$method) = array_slice($urlComponents,0,3); // simply listing out the parameters seems fine to me here.

    $params = array_slice($urlComponents,3); // we know that params is anything after the 3rd index(0 based)

    $c= '\\'.$module.'\\'.$controller; // assuming module is the namespace.

    $response = call_user_func_array([(new $c),$method],$params); // assuming your controller methods return a response, instead of just echoing it.

}else{

    throw new Exception('Cannot call method because of missing values for url: ' . $requestUrl);

}
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.