The Router - dispatch after parseURL

I'm hoping that the more experienced devs here can help me improve these methods. I'm building a basic MVC framework for myself (as a learning project,) and I'd really appreciate your insights. Things I'm looking for specifically are:

• Efficiency: can these methods be improved (particularly with the Request:parseURI() method.)?

• Clarity: do these methods make sense the way they're written, or should I refactor?

I'm including two key methods of my request routing system:

• RequestRouter

• Request

RequestRouter::dispatch()

```
public function dispatch()
{
try
{
// Instantiate the requested controller class
$class = ( $this->getController() != null ) ? ucwords( $this->getController() ) . 'Controller' : BaseController::ERROR_CONTROLLER;

/**
* Ensure this is a class that's contained in the Manifest get_class(..,true);
* If it doesn't exist in the manifest, it will not be executed.
*/
$ref = new ReflectionClass( ( class_exists($class, true) ) ? $class : BaseController::ERROR_CONTROLLER );

/**
* Ensure that the class follows conventions:
* 1. Controller class must implement IController
* 2. Controller class must implement a Controller::index() method -- this is implied since
* the first condition won't be satisfied unless the IController interface is implemented.
*/
if( $ref->implementsInterface('IController') )
{
// Instantiate a new Controller
$controller = $ref->newInstance( $this->registry );

// Check to see if the method exists
if( !$ref->hasMethod( $this->getAction() ) ) trigger_error("Method, \"{$this->getAction()}\" does not exist.", E_USER_WARNING);

// Get a ReflectionMethod object
$method = $ref->getMethod( ( $this->getAction() != null && $ref->hasMethod( $this->getAction() ) ) ? $this->getAction() : BaseController::DEFAULT_METHOD );

Large wall of text incoming. Go to the bathroom, grab a drink, etc...

Try/Catch

The first thing I would refactor is your try/catch statement. Its just way too large, though that may just be the internal comments (which I'll get to next). Try blocks should really only encompass those statements that should be try'd. Everything else is just unnecessary overhead. I would help you here, but I'm unsure what is throwing exceptions, that and I've just never been all that good with try/catch myself. Something I've been meaning to rectify.

Comments

While this isn't a functional concern, it is a legibility one. You have way too many internal comments. Even one internal comment is too many. If you have something you need to say, that your code doesn't already say, you should say it in the doccomments where it will do some good. Doccomments are defined like so:

/** Short Description
 *
 * Long Description
 * @param
 * @return
*/
public function dispatch() {

You'll notice that some of your multiline internal comments follow this syntax as well. That is because you are using the wrong multiline comments. The proper way to do internal multiline comments is like so:

/*
This is a multiline comment
*/

Though, as I've already mentioned, the key is to remove all internal comments. I'm just pointing this out for completeness.

The difference is in how they start and how they are structured. The doccomment uses two asterisks in the opening, whereas the internal one uses just one. Another difference, though not truly distinguishable between doccomment and non-doccomment, is the "bullet" asterisks on each newline. This isn't a difference between the two, merely a difference in preference. You can use "bullet" asterisks on both, not at all, or mix them. Personally, when I absolutely have to use internal comments, I use the above styles as it is immediately visually distinct from each other. Another key benefit here is that doccomments are supposed to show structure, thus "bullets", whereas regular comments should just show text (makes sense to me). The last difference are those doccomment "tags" (@param, @return). There are many and it is beyond the scope of this question to go into them here. Suffice it to say, these really help in your IDE when you are trying to use a particular method or property. Along with the autocomplete, a good IDE will also show the doccomments in some intelligent way, making it clear if that is truly the element you wished to use. Good examples of this are those doccomments associated with native PHP functions.

One key thing I mentioned at the beginning of this section, but would like to emphasize here. If your code is expressive enough, comments become less necessary. This is the reason why Self Documenting code is so commonly used. Don't use comments to explain something your code is expressive enough to explain by itself.

Ternary

I just finished explaining this in another post. This is all too common a problem, especially for those who are just learning ternary. Ternary is a very powerful tool. But you should know when to use it, and when not. This very first ternary statement is an example of a bad ternary statement. When your statements start to get too long, or too complex, or you start trying to nest them, then you should revert to using if/else statements. This ternary statement is both too long and too complex. Though, that's not to say we should immediately jump to using if/else statements. With some minor tweaking we can fix this to be a perfect ternary statement. The easiest way to do this would be to abstract our sections and simplify it.

$controller = $this->getController() . 'Controller';
$controller = ucwords( $controller );
$class = $controller == 'Controller' ? BaseController::ERROR_CONTROLLER : $controller;

So the first thing I did was I moved your getController() call out of the ternary and into a variable. This does a couple of things. First, it makes the return reusable so that you don't have to call the function again, thus not violating the "Don't Repeat Yourself" (DRY) Principle. Second, it makes it easier to check and manipulate it, as we'll do next.

In the same definition, I went ahead and appended "controller" to the end of our variable. If it turns out that getController() returns null, then all that will be left will be "controller", which is easy enough to check. Following this, in a new definition of the same variable, I capitalized the words. Notice, I did not do this in the first definition as it would have made this more difficult to read. Moving this to its own statement improves legibility and is a trick that can be used many times when refactoring, not just refactoring ternary.

There, now we can crunch the ternary like before and it is much cleaner and easier to follow. The only difference is the comparison we are using, checking for "Controller" instead of NULL, and the lack of parenthesis, they were unnecessary. Speaking of

StackExchange Code Review Q#16507, answer score: 5