5
\$\begingroup\$

I'm writing yet another MVC framework just for learning purposes and I would like you to review my code. Whenever I'm coding, I have that feeling that my code is horrible and is not the most efficient way to achieve something.

Right now the request class takes a URL path from ORIG_PATH_INFO or from PATH_INFO and explodes it into segments. Then I can easily retrieve controllers/actions/parameters from within my router class.

request.php

<?php
namespace framework\core;

class Request
{   
/**
 * Stores requested page.
 * 
 * @var array
 */
private $segments = array();

/**
 * Get requested page and explode it to segments.
 * 
 * @param string $protocol
 */
public function __construct($protocol)
{   
    if($protocol != 'PATH_INFO' and $protocol != 'ORGI_PATH_INFO'){

        throw new InvalidArgumentException('URI protocol was not setup correctly.');
    }

    $uri = (isset($_SERVER[$protocol])) ? $_SERVER[$protocol] : '';
    $this->segments = explode('/', rtrim($uri, '/'));
    array_shift($this->segments);
}


/**
 * Return all segments.
 * 
 * @return array
 */
public function getAll()
{   
    return $this->segments;
}

/**
 * Get requested controller.
 * 
 * @return mixed
 */
public function getController()
{
    if(isset($this->segments[0])){

        return strtolower($this->segments[0]);

    }else{

        return false;
    }
}

/**
 * Get requested action.
 * 
 * @return mixed
 */
public function getAction()
{
    if(isset($this->segments[1])){

        return strtolower($this->segments[1]);

    }else{

        return false;
    }
}

/**
 * Get requested parameters.
 * 
 * @return mixed
 */
public function getParams()
{
    if(isset($this->segments[2])){

        return array_slice($this->segments, 2);

    }else{

        return false;
    }
}
}

And now the router class which matches controllers/methods automatically. (Right now it's pretty basic, but later I'm planning on adding predefined routes and maybe REST)

router.php

<?php

namespace framework\core;

class Router
{   
/**
 * Prefix which will be appended to the class method.
 * 
 * @var constant 
 */
const actionPrefix = 'action';

/**
 * Router configuration data.
 * 
 * @var array 
 */
private $config = array();

/**
 * Request class object.
 * 
 * @var object
 */
private $request;

/**
 * Controller to be called.
 * 
 * @var string
 */
private $controller;

/**
 * Action to be called.
 * 
 * @var string
 */
private $action;

/**
 * Parameters which will be passed to
 * the controller.
 * 
 * @var array
 */
private $params = array();

/**
 * Store configuration and request object.
 * 
 * @param array $config
 * @param \framework\core\Request $request
 */
public function __construct($config, Request $request)
{
    $this->config = $config;
    $this->request = $request;
}

/**
 * Match url to controllers/actions and pass parameters if available.
 */
public function dispatch()
{

    $this->validateController();
    $this->validateAction();

    if(!$this->controllerExists() || !$this->actionExists()){

        require_once(APP_PATH . 'error' . DS . 'error_404.php');
        exit();
    }

    $controller = 'application\\controllers\\' . $this->controller;

    $obj = new $controller;

    if(!$this->paramsExists()){

        call_user_func(array($obj, $this->action));

    }else{

        call_user_func_array(array($obj, $this->action), $this->params);
    }  

}

/**
 * Check if user requested specific controller, if not
 * then load the default one.
 * 
 * @return boolean
 */
private function validateController()
{
    $controller = $this->request->getController();

    if($controller != false && $controller != ''){

        $this->controller = $controller;
        return true;
    }

    $this->controller = $this->config['DEFAULT_CONTROLLER'];

}

/**
 * Check if user requested a specific action, if not
 * then load the default action.
 * 
 * @return boolean
 */
private function validateAction()
{
    $action = $this->request->getAction();

    if($action != false && $action != ''){

        $this->action = self::actionPrefix . $action;
        return true;
    }

    $this->action = self::actionPrefix . $this->config['DEFAULT_ACTION'];
}

/**
 * Check of the requested controller exists in controllers
 * directory.
 * 
 * @return boolean
 */
private function controllerExists()
{
    $path =  APP_PATH . 'controllers' . DS . $this->controller . '.php';
    if(file_exists($path)){

        return true;
    }

    return false;
}

/**
 * Check to see if action is callable or not.
 * 
 * @return boolean
 */
private function actionExists()
{   
   if(is_callable(array('application\\controllers\\' . $this->controller, $this->action))){

       return true;
   }

   return false;
}

/**
 * Figure out if we have to pass additional parameters
 * to the requested controller.
 * 
 * @return boolean
 */
private function paramsExists()
{
    if($this->request->getParams())
    {
        $this->params = $this->request->getParams();
        return true;
    }

    return false;

}

}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

From quick glance I see a lot of logic in the constructor, usually not recommended.

I would also avoid hard-coded indices for segments and use regular expressions to parse the URLs. Basically the less hard-coded strings are in the code, the better.

You can place them together with all your constants and settings in a separate Config class or JSON. Then you can quickly change those settings and the code becomes more re-usable.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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