7
\$\begingroup\$

On my way to learn OOP, I am developing a little CMS using the MVC pattern. I would like to have some feedback about my router class, mostly about if I am correctly using OOP.

Right now, I have a .htaccess file redirecting requests to an index.php file, both of them in the root directory of the project.

.htaccess

<IfModule mod_rewrite.c>
RewriteEngine on
RewriteCond %{REQUEST_FILENAME} !\.(jpg|jpeg|gif|png|css|js|pl|txt|otf|ttf|eot|woff|woff2|svg)$
RewriteRule ^(.*)$ index.php?uri=$1 [QSA]
</IfModule>

Then I run this router class to get the requested URL and call to the correct controller class and method:

Router.php

<?php
namespace Core;

use Controller;

if ( !class_exists( 'Router' ) ) {

    class Router {

        /**
        *   Allowed URL patterns
        * @var Array
        */
        private $routes;

        /**
        *   Allowed prefixs for URLs
        * @var String
        */
        private $prefix;

        /**
        *   Requested URL
        * @var String
        */
        private $url;

        /**
        *   Run the router
        */
        public function run() {
            $this->set_url();
            $this->dispatch();
        }

        /**
        *   Set a new route
        */
        public function set($route) {

            if ( is_array($route) ) {
                foreach ( $route as $regex => $control ) {
                    $this->routes[$regex] = $control;
                }
            } else {
                $default_routes = $this->generate_default_routes($route);
                foreach ( $default_routes as $regex => $control ) {
                    $this->routes[$regex] = $control;
                }
            }

        }

        /**
        *   get routes
        */
        public function get_routes() {

            return $this->routes;

        }

        /**
        *   Default routes
        */
        public function generate_default_routes($object) {
            ucfirst($object);
            return $routes = array(
                '~^\/'.$object.'s\/$~'               => $object.'sController@list_all',
                '~^\/'.$object.'\/(\d+)\/$~'         => $object.'sController@display',
                '~^\/admin\/'.$object.'s\/$~'               => $object.'sController@list_all',
                '~^\/admin\/'.$object.'\/new\/$~'           => $object.'sController@create',
                '~^\/admin\/'.$object.'\/(\d+)\/edit\/$~'   => $object.'sController@edit',
                '~^\/admin\/'.$object.'\/(\d+)\/delete\/$~' => $object.'sController@delete',
            );
        }

        /**
        *   set routes prefix
        */
        public function set_prefix($prefix) {

            foreach ( $prefix as $regex => $control ) {
                $this->prefix[$regex] = $control;
            }

        }

        /**
        *   get requested url
        */
        public function set_url() {

            $url = isset($_GET['uri']) ? '/' . $_GET['uri'] . '/' : '';
            $this->url = urldecode(trim($url));

        }

        /**
        *   get requested url
        */
        public function get_url() {

            return $this->url;

        }

        /**
        *   Call the correct controller class and method
        */
        public function dispatch() {

            //find if the url match any specified route
            $args = $this->find_match($this->url, $this->routes);

            //set data
            $action = isset($args['action']) ? $args['action'] : '';
            $id = isset($args['id']) ? $args['id'] : '';
            $admin = isset($args['admin']) ? $args['admin'] : '';

            if ( !empty($action) ) {

                if ( preg_match('/@/', $action) ) {

                    list($object, $method, $params) = $this->get_call_params($action, $id, $admin);
                    call_user_func_array(array($object,$method), $params);

                } else {
                    echo "invalid method supplied for the route: " . $this->url;
                }

            } else {
                echo "invalid route: " . $this->url;
            }

        }

        /**
        *   find if the requested URL match any of the allowed route patterns
        */
        public function find_match($uri, $routes) {

            $args = array();

            foreach ( $routes as $regex => $controller) {
                if ( preg_match($regex, $uri) ) {
                    $args['action'] = $controller;

                    if ( preg_match('/([a-zA-Z]+)\/(\d+)/', $uri) ) {
                        preg_match('/(\d+)/', $uri, $matches);
                        $args['id'] = $matches[0];
                    }

                    if ( preg_match('~^\/admin/~', $uri) ) {
                        preg_match('~^\/admin/~', $uri, $matches);
                        $args['admin'] = $matches[0];
                    }
                }
            }

            return $args;

        }

        /**
        *   Prepare data for correct dispatching
        */
        public function get_call_params($action, $id, $admin) {

            //split action into controller and method
            $actions = explode('@', $action);
            list($controller, $method) = $actions;

            if ( !empty($admin) ) {
                $prefix = $this->prefix['admin'];
            } else {
                $prefix = $this->prefix['default'];
            }

            $controller = ucfirst($controller);

            //instantiate the controller object
            $class = 'Controller\\' . $controller;
            $object = new $class($action, $prefix);

            //set params array
            $params = array();
            $params[] = $id;

            return array($object, $method, $params);

        }


    }

}


?>

I use this routes.php file as routes configuration file, where I set the allowed paths objects for the paths patterns and the prefixes.

routes.php

<?php

/**
 * Allowed url pattern prefixes
 */
$router->set_prefix( array( 'default'   => '' ) );
$router->set_prefix( array( 'admin'     => 'admin' ) );


/**
 * Set up allowed URLs
 */
$router->set( array ( '/\//'    => 'BaseController@index' ) );
$router->set( array ( '/login/' => 'Auth@login' ) );
$router->set( array ( '/logout/'    => 'Auth@logout' ) );
$router->set( array ( '/admin/' => 'BaseController@admin_index' ) );
$router->set('page');
$router->set('user');
$router->set('cat');
$router->set('tag');
$router->set('media');
$router->set('option');

?>
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Doing if ( !class_exists( 'Router' ) ) is bad for opcode caching. Instead use require_once to only load the php file that defines the class once. \$\endgroup\$ Jan 13, 2016 at 15:05

1 Answer 1

3
\$\begingroup\$

Don't repeat yourself

    public function set($route) {

        if ( is_array($route) ) {
            foreach ( $route as $regex => $control ) {
                $this->routes[$regex] = $control;
            }
        } else {
            $default_routes = $this->generate_default_routes($route);
            foreach ( $default_routes as $regex => $control ) {
                $this->routes[$regex] = $control;
            }
        }

    }

can and should probably be written without duplicated logic :

        $routes = is_array($route) ? $route : $this->generate_default_routes($route);
        foreach ( $default_routes as $regex => $control ) {
            $this->routes[$regex] = $control;
        }

Similarly :

        if ( !empty($admin) ) {
            $prefix = $this->prefix['admin'];
        } else {
            $prefix = $this->prefix['default'];
        }

could become :

        $prefix = $this->prefix[empty($admin) : 'default' : 'admin'];

Style

You are a bit inconsistent in your vertical spacing. My personal point of view is that you don't need that many blank lines.

\$\endgroup\$

Your Answer

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

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