HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Creating an object-oriented router class in PHP

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
creatingphprouterorientedobjectclass

Problem

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


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]


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

Router.php

```
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\/$~

Solution

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.

Code Snippets

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

    }
$routes = is_array($route) ? $route : $this->generate_default_routes($route);
        foreach ( $default_routes as $regex => $control ) {
            $this->routes[$regex] = $control;
        }
if ( !empty($admin) ) {
            $prefix = $this->prefix['admin'];
        } else {
            $prefix = $this->prefix['default'];
        }
$prefix = $this->prefix[empty($admin) : 'default' : 'admin'];

Context

StackExchange Code Review Q#116571, answer score: 3

Revisions (0)

No revisions yet.