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 which, try using is_null() in the future, its a bit more obvious, or at the very least use an absolute comparison !==. Just using a loose comparison != means it will match anything "similar" to NULL, which could just as easily be rewritten using the subject of the comparison as a boolean:
$this->getController() != null
//is the same as
$this->getController()
I'm not going to do this for each of your ternary statements, but just know that the above will be helpful for all of them. One last tip here: abstract your ternary from other statements. Ternary, should be its own statement and should not be part of a larger one, as you have done in the next ternary statement.
DRY
I mentioned DRY above, but I found another, more obscure, violation. So, here is a section just for it. As the name implies, you don't want your code to repeat itself. Good code is completely reusable, either through functions/methods or variables/properties or loops. So, the fact that you are using BaseController::ERROR_CONTROLLER three separate times should be a hint that you should refactor it somehow. The easiest way would be to assign it to a local variable or property. This makes it easier to change if you ever decided to do so. Another benefit is shorter code. But its not all just variables/properties and functions/methods and loops. Sometimes you can just slightly refactor your code to eliminate the need for something. For instance, in your two ternary statements you use that constant in case of an error. Since you are handling that error the same both times, why not just combine the ternary and check it once?
$error = BaseController::ERROR_CONTROLLER;
$classDefined = $controller != 'Controller'
$classExists = class_exists( $class, true );
$class = $classDefined && $classExists ? $controller : $error;
$ref = new ReflectionClass( $class );
Single Responsibility Principle
As with the try/catch block, you can just tell by looking at this method that it is too large. Initially I just thought it was due to the excessive internal comments, so I didn't mention it. However, upon closer inspection it is still true. If we follow the Single Responsibility Principle, then we know that our functions/methods should do one thing, to the exclusion of all others. And that one thing should be easily identifiable by its name. In this case, dispatch() should be a conglomeration of other method calls, used to "dispatch" the request. Instead, you are, for the most part, doing everything here and delegating nothing. For instance, everything I've covered up to this point could be considered part of a validateController() method. I'll leave the rest up to you, it should be relatively easy. Just try grouping your code by functionality (note the root word function).
Troublesome Syntax
PHP inherently requires braces {} on its expressions, otherwise you wouldn't have to add them after adding more than one statement. I've had this argument with another member (Corbin I think), and though he agrees with my sentiments he says that this is common and not an incomplete implementation, however, I stand by this. If PHP were to implement entirely braceless syntax I would not comment on someone using it, so long as they were consistent. But the half-assed implementation that they do have just promotes bad habits and should be avoided. It is inconsistent and prone to errors. A new user picking up this code, not understanding this syntax, would add additional lines in such a statement and, unwittingly, crash the program. If they are lucky they will be able to figure this out quickly, but more than likely this wont have been the only thing they changed and it will quickly become frustrating or troublesome. So, in short, it is always best to use braces on your expressions, even those one liners. I'd also suggest adding a newline after the expression to help with legibility and line length, but that is a preference.
if( !$ref->hasMethod( $this->getAction() ) ) {
trigger_error("Method, \"{$this->getAction()}\" does not exist.", E_USER_WARNING);
}
Foreach Loops
I'm a bit confused as to if this is just a typo, or if you are just unsure about this syntax. With foreach loops you can access either the $key => $value pair, or just the $value. In the following snippet you are requesting the key/value pair with $route as your key, but then never using the key. After that you then use the alternate syntax as prescribed. Since the key isn't being used in either loop, both foreach loops should follow the syntax of the second loop.
foreach( $pattern_collection as $route => $patterns )
{
foreach( $patterns as $pattern )
Bonus: Some times it is desirable to only loop over the keys with a foreach loop. Many times programmers use the key/value pair syntax to do this when, in fact, the proper syntax is to use array_keys() on the array and loop over that instead.
Conclusion
Whew, done. There's quite a bit here, but hopefully I covered everything. If you have any questions, just let me know. I wasn't able to review your MVC implementation because all you provided was key methods rather than full classes. Which is fine, but typically one of the biggest things most programmers have issues with when programming MVC frameworks for the first time is the class structure. For instance, they may get a controller and model mixed up, or combine the controller and view (though in some special cases this is permissible). One last parting comment: This is a really complex MVC you have planned out. Typically a first MVC framework will not use the reflection class as the developer will hardcode any functionality they need, not worrying about adding dynamic method calling until they understand the structure more. You've not just jumped into the deep end, you've jumped off the cruise ship into the middle of the ocean. Good luck to you and I hope you can swim :)
