trjhtr

I don't know if you guys get tired of seeing a bunch of requests for review of similar projects, but I was really looking for advice.

My website is designed on the MVC pattern. I have a very simple router that seems to be working just fine! If there is any upgrade I would add to it would be the ability to catch a third parameter (optional ID number, for example).

Any thoughts on the design/execution would be greatly appreciated.

Here is the Router class:

namespace Framework

 use AppControllers;

 class Router
 
 private $_url = ;

 public function addRoute($route, $dest)
 
 $this->_url[$route] = $dest;
 

 public function redirect()
 
 $url = isset($_GET["url"]) ? $_GET["url"] : "";

 foreach ($this->_url as $route => $dest)
 
 if("/$url" == $route)
 
 $this->parsecommand($dest);
 break;
 
 
 

 private function parsecommand($dest)
 
 $goto = explode("@", $dest);

 $controller = $goto[0];
 $action = $goto[1];

 require_once("../app/controllers/$controller.php");

 $class = '\App\Controllers\' . $controller;
 $obj = new $class;

 return $obj->$action();
 
 

The routes are entered this way:

namespace App

 use FrameworkRouter;

 $routes = new Router;

 $routes->addRoute("/", "HomeController@index");
 $routes->addRoute("/aboutus", "HomeController@aboutus");

 $routes->addRoute("/blog", "BlogController@index");

 return $routes;

...which runs as a part of a bootstrap process called from index.

And my controllers look like this:

namespace AppControllers

 use AppController as Controller;
 use AppModelsUpdate as Update;

 class HomeController extends Controller
 

 public function index()
 
 $this->view('index', [
 'update' => Update::orderBy('id', 'desc')->first(),
 ]);
 

 public function aboutus()
 
 $this->view('aboutus');
 
 

Rendering the views is handled by Twig in the Controller base class (not really relevant to this question).

Like I said, this works perfectly (for my needs, I wouldn't market it or anything, haha), just wondering if I could get some advice.

EDIT: This does not handle 404 (or any other error for that matter), I realize that would probably the first thing to upgrade.

Welcome to code review! Don't worry, we don't get tired of these things (or at least I don't). In retrospect I would have loved to have something like this in my early days - and it's still useful to me now even though I'm well past my early years.

I'll just start from small items and work my way up. To be fair, I'm a stickler for the little things. You'd be surprised how often the little things can make a big difference in the long run, especially when you are talking about the core part of a system (such as this). So let's start with the little stuff:

1. You don't have to use braces and indent after a namespace. This is actually the only place I suggest not indenting or including braces. The reason is because it simply pushes the indentation of your entire file up one level unnecessarily. Just end your namespace declaration with a semi-colon and leave the use lines and class definition without indentation.


2. The usage of the redirect method in your router is unclear, and as such I'm not even 100% sure if your redirect method even belongs in the router. That sounds more like something that should be part of the object that manages the HTTP response (which would also handle things like 404 errors and what not). Then again it seems like the only part that actually does routing, so maybe the issue is your name. Perhaps it should be called route()? That makes sense to me: in your index file you would call $router->route() and expect a controller to be executed.


3. In the redirect method you use a loop over the routes, which is costly (in terms of performance). A better option would simply be to do a lookup via isset. if (isset($this->_url[$url])) .


4. You have no handling of error conditions. What happens if someone tries to redirect to a route that doesn't exist? What happens if there is no @ in the route? It is best to explicitly check for such situations and throw an exception so that you can provide clear details to the developer.


5. Similar, enforcing some consistency in your data can be helpful. Your router obviously assumes that routes start with a slash, so you should enforce that in the addRoute method. This will take care of a whole class of subtle but hard-to-identify bugs down the road.


6. Your names are a bit inconsistent. See addRoute vs parsecommand. Camel case for one but all lowercase for the other. Pick a convention and stick to it. If in doubt use PSR.


7. Your variables names could use a bit more consistency too. Instead of using a private variable named _urls in your router, I think routes would make much more sense. Also (this could be a matter of preference), you don't really need the leading underscore in the name of private variables. IMO, that's a convention that primarily belongs in languages that don't have enforced visibility rules.


8. 
   Writing docblocks will help a lot in the long run.


9. Your parsecommand method returns whatever the controller returns, but the redirect method ignores the return value. This makes sense because your controllers don't actually return anything. If the controllers will always print directly, then there is probably no reason for paresecommand to return anything.

Some larger suggestions:

Dependency Injection:

As I mentioned before, you don't really want to access the $_GET super global (or other super globals) directly. It is better to use a separate class to get that information out. It all boils down to separation of concerns. As a simple example of where this can break down, how would your current setup handle things if you decided to add a command line component? Perhaps that would have a completely separate router, but perhaps you would rather use the same system you already have in place. In that case your current setup simply won't work - it is hard-coded to look at $_GET, which doesn't exist in the command line environment. Instead you could have your router expect to receive a request object in the constructor, and that request would tell it the requested URL. In the case of a normal web request the request object would simply get the URL out of the $_GET super global, but for a command line request you would perhaps send in a different request object that gets the url out of the command line arguments.

This all boils down to the concept of dependency injection - instead of directly creating/managing your own dependencies, you let the person in charge of you give the necessary dependencies. This makes the code much more flexible. It also allows you to create integration and unit tests, which is otherwise impossible when you are directly hooked into the request itself. For the moment of course I wouldn't suggest switching completely over to managing your system with a full dependency injection container. However, if you get in the habit of separating out your services a bit more and (in this example), expecting a request object to come in with the constructor, you'll start getting in much better habits. While you are keeping things simple, now is the time to start learning about dependency injection and test writing.

Autoloading

In your router you require the controller PHP file directly. This implies that you don't have autoloading setup. Setting that up in your bootstrap process will save you a lot of trouble. For instance, your current setup requires your controllers to all live in the same directory. With an autoload setup that maps namespaces to file directories, you can remove that requirement and make it easier to organize your app. Then you could do things like:

$routes->addRoute('/shopping/products', 'ShoppingProductController@index');
$routes->addRoute('/support/contact', 'SupportContactController@index');

Presumably your files would then be located in (something like) Controllers/Shopping/ProductController.php, Controllers/Support/ContactController.php, etc... This will help you keep your app organized better if it starts growing larger than a handful of controllers.

Performance

I don't think this is either a surprise or a problem for you, but just to say out loud, this setup will start to cause (performance) trouble if you start to add dozens/hundreds of endpoints. It doesn't sound like that is your intention, but for something like this to scale to large apps you may need a different routing mechanism or some caching mechanism (or a completely different solution).

That's what I have for now! Let me know if you have any followup questions/comments.