PHP Route class

Question

I was trying to keep it SRP. I'm new into PHP OOP and I'm wondering how I can make it better.

Route.php

class Route
{
 private $_url;
 private $_callback;
 private $_name;
 public function __construct($_url, $callback)
 {
 $this->_url = '/^' . str_replace('/', '\\/', $_url) . '$/';
 if (is_callable($callback)) {
 $this->_callback = $callback;
 return;
 }
 if (is_string($callback) 
 && substr_count($callback, '@') == 1 
 && file_exists(CONTROLLER_PATH . '/' . explode('@', $callback)[0] . '.php')) {
 $this->_callback = explode('@', $callback);
 require_once CONTROLLER_PATH . '/' . $this->_callback[0] . '.php';
 $this->_callback[0] = new $this->_callback[0];
 return;
 }
 exit('Error!');
 }
 public function getURL()
 {
 return $this->_url;
 }
 public function getCallback()
 {
 return $this->_callback;
 }
}

Router.php

<?php
class Router
{
const GET = 'GET';
const POST = 'POST';
private static $_routes = array(
 'GET' => array(),
 'POST' => array()
);
public static function add(Route $route, $method)
{
 switch ($method) {
 case 'GET':
 self::$_routes['GET'][$route->getURL()] = $route->getCallback();
 break;
 case 'POST':
 self::$_routes['POST'][$route->getURL()] = $route->getCallback();
 break;
 default:
 exit('Error!');
 break;
 }
}
public static function get(Route $route)
{
 self::add($route, self::GET);
}
public static function post(Route $route)
{
 self::add($route, self::POST);
}
public static function run()
{
 $path = array_key_exists('path', $_GET) ? '/' . $_GET['path'] : '/';
 foreach (self::$_routes[$_SERVER['REQUEST_METHOD']] as $url => $callback) {
 if (preg_match($url, $path, $matches)) {
 array_shift($matches);
 call_user_func_array($callback, array_values($matches));
 return;
 }
 }
}
}

controller/HomeController.php

<?php
class HomeController
{
 public function index()
 {
 echo 'Hello World!'
 }
}

Using example

<?php
Router::get(new Route('/', 'HomeController@index'));
Router::run();
?>

• Mathieu Guindon: Welcome to Code Review / Stack Exchange! Please edit your title to simply state what problem your code is solving; every single question on this site is asking about "code improving" ;-) ...also feel free to include more of the surrounding context code, for example a whole method is easier to review than just an if block taken out of context. See how to get the best value out of Code Review on meta for more info.
• Marc-Andre: You're edit made the question way better! I hope you'll have good reviews.
• tim: Can you add a usage example? I think it's a bit confusing how callback is used, and url isn't used at all, which adds to the confusion.

Answer 1

In general your code is a good start. Keep going :)

However the class Route should be refactored.

Issuelist:

• Missing setter
• In the constructor are multiple returns
• Script is terminated in the Route class
• The if-conditions are not well-readable

Description:

This section describes missing setter, multiple returns and if-conditions.

In the constructor of the class Route are multiple returns. This makes the code less maintainable. Having more than one return means there are multiple scenarios when the constructor can be stopped. In case of a bug one need to debug through the whole method to figure out the return-point.

So, at this point a switch to if-else-if-else should be performed. Below you see a minimalistic sample.

<?php
if (is_callable($callback))
{
 // simple assignment.
}
else if (is_string())
{
 // parsing, then assigning
 }
 else
 {
 exit();
 }

As you notice it is better readable. From here there are two approaches how the code can be optimized further.

Approach 1:

IsValid-methods should be created. These kind of methods performs a validation.

<?php
public function IsValidCallback($callback)
{
 // Validate $callback
}

The IsValidCallback-method may check is_callable as well.

The constructor looks like that now:

<?php
if ($this->IsValidCallback($callback))
{
 if (is_callable($callback))
 {
 // simple assignment
 }
 else
 {
 // parsing and assigning
 }
 }
 else
 {
 exit();
 }

The code became even better readable and when the validation has to be adjusted it is obvious where to make the changes plus you can validate the Route at any line in your software with the guarantee the validation is up to date.

Approach 2:

Move the assignment to a respective setter and throw an InvalidArgumentException.

<?php
protected function _setCallback($callback)
{
 $callback = (string) $callback;
 $aCallback = explode('@', $callback);
 if (is_callable($callback)) {
 $this->_callback = $callback;
 }
 else if (count($aCallback) == 2 && file_exists(CONTROLLER_PATH . '/' . $aCallback[0] . '.php'))
 {
 require_once CONTROLLER_PATH . '/' . $aCallback[0] . '.php'; // instead of including the class you can make usage of http://php.net/manual/en/function.spl-autoload-register.php
 $this->_callback = new $aCallback[0];
 }
 else
 {
 throw new InvalidArgumentException('$callback is invalid.');
 }
}
public function __constructor(...)
{
 try
 {
 $this->_setCallback($callback);
 }
 catch(InvalidArgumentException $e)
 {
 exit();
 }
 }

Script is terminated in the constructor

The script should not be terminated in a random class but by an ExceptionHandler. The type of the exception decides wether the script has to be terminated. Commonly a FatalException leads to a termination.

Possible Route class

class Route
{
private $_url;
private $_callback;
private $_name;
// REFACTORED
public function __construct($_url, $callback)
{
 $this->_setURL($_url);
 try
 {
 $this->_setCallback($callback);
 }
 catch(InvalidArgumentException $e)
 {
 exit('Error!');
 }
}
// ADDED
protected function _setCallback($callback)
{
 $callback = (string) $callback;
 $aCallback = explode('@', $callback);
 if (is_callable($callback)) {
 $this->_callback = $callback;
 }
 else if (count($aCallback) == 2 && file_exists(CONTROLLER_PATH . '/' . $aCallback[0] . '.php'))
 {
 require_once CONTROLLER_PATH . '/' . $aCallback[0] . '.php'; // instead of including the class you can make usage of http://php.net/manual/en/function.spl-autoload-register.php
 $this->_callback = new $aCallback[0];
 }
 else
 {
 throw new InvalidArgumentException('$callback is invalid.');
 }
}
// ADDED
protected function _setURL($url)
{
 $this->_url = '/^' . str_replace('/', '\\/', $url) . '$/';
}
public function getURL()
{
 return $this->_url;
}
public function getCallback()
{
 return $this->_callback;
}
}

• HeuHeu: Thank you so much, I have waited so long for answer like this! :)
• Alexander: @HeuHeu I have updated the answer.
• HeuHeu: Approach 1.: Everything is clear, thanks! :) Approach 2.: 1. What is the purpose of $blIsValid?? 2. What if we pass Closure as $callable?? The InvalidArgumentException will be thrown. But Closure is also a valid callback for this class (I think). And I don't really understand the "Script is terminated in the constructor" section. Can you show me an example of refactoring class that way?? Thanks a lot! :)
• Alexander: Good to hear. Regarding the second approach: I have returned $blIsValid in an earlier version of the method. In the latest answer it is removed. What do you mean with "Closure"? "Script is terminated in the constructor": You are using the core-method die in the constructor which terminates the software. Sure, going to do this now.
• Alexander: I am not using an anomyous function in my approaches. I have added a sample route class. Keep in mind that this is not the best solution. You can make usage of an autoloader for example. You can find more about it on php.net Despite of that think about SRP while having a look at your and my code. Reminder: Both a class and a method should solve one task only and it's naming has to describe clearly what it does.