Krdytkyu

Before I start writing my first MVC framework, I want to improve my code quality as much as I can, can anyone give me some constructive criticism on how I can improve this?

It is a simple router, built for simple use. It should be pretty simple what's going on, and the usage should be easy to picture. It's nothing major but works well for simple functionality using it.

<?php



class Router {



    public $routes = ;

    public $routeMethods = ;



    public function get($route, $settings) {

        $this->add($route, $settings, ['GET']);

    }



    public function post($route, $settings) {

        $this->add($route, $settings, ['POST']);

    }



    public function add($route, $settings, $methods) {

        $this->routes[$route] = $settings;

        $this->routeMethods[$route] = $methods;

    }



    public function dispatch() {

        $requestUrl = $_SERVER['QUERY_STRING'];



        if (array_key_exists($requestUrl, $this->routes)) {

            $route = $this->routes[$requestUrl];

            $methods = $this->routeMethods[$requestUrl];

            $method = $_SERVER['REQUEST_METHOD'];



            if (!in_array($method, $methods)) {

                echo 'Method ' . $method . ' not allowed.';

            }

            else {

                $controller = $route['controller'];



                if (class_exists($controller)) {

                    $class = new $controller(); // grab from di cache when we have one

                    $action = $route['action'];



                    if (method_exists($class, $action)) {

                        $class->$action();

                    }

                    else {

                        echo 'We couldn't find action '' . $action . ' in controller '' . $controller . ''';

                    }

                }

                else {

                    echo 'We couldn't find controller '' . $controller . ''';

                }

            }

        }

        else {

            echo '404 not found';

        }

    }



}

• set public $routes to private or protected if its jsut an internal resource of this class. Just use public if other classes/code explicitly should call the mothod/property


• 
  method_exists doesn't check if the method is callable. But that might be important if you execute legacy code


• Avoid echo 'some text' Better use exceptions and an exception handler


• If you use if (){} else{} you could think about using the if for the termination condition and the function continues regulary if its ok


• Your note about the dependency injection cache is good. Keep your idea. You are on the rigth way.


• I would recommend to use doc-blocks as annotation and for IDE autocompletion for all public functions as well as class properties. This will increase the readability


• You didn't said which version of PHP you are using. Switch to >= 7.1 because 7.0 will reach its end of life soon. So you should use return types and type hinting

Expanding on the answer provided by unherz;

Validate the programmer

Sometimes we make mistakes so you should check the $settings provided by the programmer are correct as you require them to pass at least two parameters action & controller so I would add a method validateRouteSettings I also implemented unherz answer to show how early returns (exceptions in this case) would look

Naming

You call object methods "actions" which is in-correct, should try to be consistent in naming structures

Any way here an updated implementation

<?php



class Router {



    protected $routes = ;

    protected $routeMethods = ;



    public function get(string $route, array $settings) :void {

        $this->add($route, $settings, ['GET']);

    }



    public function post(string $route, array $settings) :void {

        $this->add($route, $settings, ['POST']);

    }



    public function add(string $route, array $settings, array $methods) :void {

        $this->validateRouteSettings($route, $settings);

        $this->routes[$route] = $settings;

        $this->routeMethods[$route] = $methods;

    }



    private function validateRouteSettings(string $route, array $settings) :bool

    {

        if(!isset($settings["controller"])){

            throw new Exception("Missing controller for $route", 1);

        }elseif(!isset($settings["method"])){

            throw new Exception("Missing method for $route", 1);

        }



        return true;

    }



    public function dispatch() :void {

        $requestUrl = $_SERVER['QUERY_STRING'];



        if (!array_key_exists($requestUrl, $this->routes)) {

            throw new Exception('404 not found', 1);

        }



        $route = $this->routes[$requestUrl];

        $methods = $this->routeMethods[$requestUrl];

        $method = $_SERVER['REQUEST_METHOD'];



        if (!in_array($method, $methods)) {

            throw new Exception('Method ' . $method . ' not allowed.', 1);

        }



        $controller = $route['controller'];



        if (!class_exists($controller)) {

            throw new Exception('We couldn't find controller '' . $controller . ''', 1);

        }



        $class = new $controller(); // grab from di cache when we have one



        $method = $route['method'];



        if (!method_exists($class, $method)) {

            throw new Exception('We couldn't find method '' . $method . ' in controller '' . $controller . ''', 1);

        }



        $class->$method();

    }

}