PHP Route class

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

 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.ph

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:

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.

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

StackExchange Code Review Q#120587, answer score: 2