patternphpMinor
Creating an object-oriented router class in PHP
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
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\/$~
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
can and should probably be written without duplicated logic :
Similarly :
could become :
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.
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.