0
\$\begingroup\$

How can I write my code more efficient (not repeating same code)?

And is there a solution that simplifies the way of having to create a lot of dependency objects over and over again whenever I instantiate a new class (as seen in the load method)?

Any suggestions are welcome.

class Router
{
    private $request;
    private $cleanUri;
    # control attributes
    private $controller;
    private $action;
    private $params = [];

    public function __construct(Request $request)
    {
        $this->request  = $request;
        $this->cleanUri = preg_replace(['#/+#', '(\.\./)'], '/', trim($request->getUri(), '/')); 

        $this->parseUri();
        $this->load();
    }

    private function parseUri()
    {
        $parts            = explode('/', $this->cleanUri);
        $this->controller = ($this->cleanUri !== '' ? ucfirst(array_shift($parts))  : 'Index') . 'Controller';
        $this->action     = array_shift($parts);
        $this->params     = !empty($parts) ? $parts : null;
    }

    private function load()
    {
        if (class_exists($this->controller)) {
            if (empty($this->action)) {
                $this->action = 'index';
            } elseif ($this->action !== 'index' && method_exists($this->controller, $this->action)) {
                //$action value stays the same
            } else {
                if (class_exists('_404Controller')) {
                    $controller = new _404Controller();

                    $controller->index();
                } else {
                    throw new Exception('404 controller not found. Page doesn\'t exist for: ' . $this->request->getUri());
                }
            }

            $controller = $this->controller;

            //create dependencies
            $db   = new Database();
            $user = new User($db, $this->request);
            $menu = new MenuHelper($this->request, $user);

            $controller = new $controller($this->request, $user, $menu);
            $action     = $this->action;

            $controller->$action();
        } else {
            if (class_exists('_404Controller')) {
                $controller = new _404Controller();

                $controller->index();
            } else {
                throw new Exception('404 controller not found. Page doesn\'t exist for: ' . $this->request->getUri());
            }
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

If this unit of code is a router, then it should remain a router. Have the controller handle its dependencies. You could have multiple controllers, each with its own dependencies. You wouldn't want a single router code become a monster just to keep up with the controllers.

class Router{

  private $request;
  private $cleanUri;

  private $controller;
  private $action;
  private $params = [];

  public function __construct(Request $request){
      $this->request  = $request;
      $this->cleanUri = $this->sanitizeUri($request->getUri())
      $this->parseUri();
      $this->load();
  }

  // Extract sanitize. You might want to add more than just this and the constructor
  // is a bad place to put it.
  private function sanitizeUri($uri){
    return preg_replace(['#/+#', '(\.\./)'], '/', trim($uri, '/'));
  }

  private function parseUri(){
      $parts            = explode('/', $this->cleanUri);
      $this->controller = ($this->cleanUri !== '' ? ucfirst(array_shift($parts))  : 'Index') . 'Controller';
      $this->action     = array_shift($parts);
      $this->params     = !empty($parts) ? $parts : null;
  }

  // Extract the 404
  private function send404(){
    if (class_exists('_404Controller')) {
      $controller = new _404Controller();
      $controller->index();
    } else {
      throw new Exception('404 controller not found. Page doesn\'t exist for: ' . $this->request->getUri());
    }
  }

  private function load(){

    // If action is empty, use index
    if (empty($this->action)) $this->action = 'index';

    // If either controller or method doesn't exist, 404
    if(!(class_exists($this->controller)  && method_exists($this->controller, $this->action))) 
      $this->send404();

    // else execute
    $controller->$action($this->params);

  }
}

Then the controller could have:

class MyController extends Controller{

  public function __construct(){
    $this->load(array(
      'database',
      'cats',
      'unicorns'
    ));
  }

  public function index(){
    $this->database->getSomething();
  }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks. I really like your answer. It makes my code more modular and more easy modifiable and understandable. But let's say I will need a User object in every of my controllers so I can check if a user is logged in or not with isLoggedIn() method. Wouldn't it be a good idea to initialize / inject it in the constructor of the controller parent class? And you also forgot to store the class property in a variable first before being able to instantiate the controller and call it's method with a variable in the load method. \$\endgroup\$ – Kid Diamond May 20 '14 at 22:04
  • \$\begingroup\$ @KidDiamond My PHP's a bit rusty so apologies. If you need a class that needs to be loaded for all modules, consider having a config file, which could contain an array. You can include them in the router and feed them to the controller. \$\endgroup\$ – Joseph May 21 '14 at 0:40
  • \$\begingroup\$ By using dependency injection on the costructor of the router (config array) and in the abstract controller (the class(es) that need to be loaded in every controller), right? \$\endgroup\$ – Kid Diamond May 21 '14 at 5:54

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.