2
\$\begingroup\$

I am currently rewriting a legacy PHP4 application to a MVC-style object-oriented one.
As the most complex part is the models, I came up with the following controller-view scheme. I wanted to use only variables in my views, using only <? if ($foo === 'bar'): ?>-like blocks if necessary. The controllers instead generate the variables for the views from the data generated by my models.

Is the following a good approach to MVC code separation?

The controller code looks like this:

interface InterfaceController {
    public function setup();
    public function compile();
}

abstract class AbstractPageController implements InterfaceController
{
    public static $variables = array();
    public static $template = '';

    public function compile() {
        static::$variables['header'] = Controller::run('partials/header');
        static::$variables['footer'] = Controller::run('partials/footer');

        return (new View(static::$template, static::$variables))->render();
    } 
}

class ExampleController extends AbstractPageController
{
    public function setup() {
        $variables['foo'] = 'bar';

        static::$template = 'examplepage';
        static::$variables = $variables;

        return $this;
    }
}

An example route would look like that:

/* ... */

Route::get('/example/supercoolfeature', function() {
    Controller::run('example/features/supercool');
});

/* ... */

So, the router calls the following Controller class:

class Controller
{
    /**
     * Executes a controller:
     * If called as "Controller::run('foo/bar')", the function will call the
     * class "App\Controllers\Foo\BarController" and execute the setup() and run()
     * methods. 
     */
    public static function run($name) { 
        $parts = array();

        foreach (explode('/', $name) as $part) {
            // uppercase each controller path part so you can
            // go lowercase in the actual code
            $parts[] = ucfirst($part);
        }

        // attach the parts together and append "Controller" at the end
        $name = '\\App\\Controllers\\' . implode('\\', $parts) . 'Controller';

        // run the controller, let it prepare a view and return the rendered code
        return (new $name())->setup()->compile();
    }
}

And finally, the respective view for the controller:

<?php echo $header; ?>
<div class="hi">
    <h1>Hi there, <?php echo $foo; ?>!</h1>
</div>
<?php echo $footer; ?>
\$\endgroup\$
  • 3
    \$\begingroup\$ Your title should be a brief statement that tells us what the code does, not a generic question. Your primary concern(s) go into the question body. \$\endgroup\$ – Nic Hartley Jun 13 '15 at 16:06
  • \$\begingroup\$ @QPaysTaxes so, prior to editing it - would "View-Controller relationship in PHP" be a better title then? \$\endgroup\$ – Moritz Friedrich Jun 13 '15 at 16:39
  • \$\begingroup\$ I think the term you want is View-Model. \$\endgroup\$ – Greg Burghardt Jun 13 '15 at 20:24
2
\$\begingroup\$

your code is not bad. You write comments that don't state the obvious, so thats good. You don't make stupid rookie mistakes like I used to make, so thats good. But... (such a lovely word)

Naming

You define an interface: InterfaceController. The name tells me that every class that implements this interface, is some kind of Controller of interfaces. It sounds fancy, I must give you that.

Maybe it's personal, but I just don't like writing implements RandomInterface. Ofcourse it's an interface, thats why I implement it. And if you have a problem where the interface and the class implementing it have the same name, one of the two should not exist.


AbstractPageController implements Controller: So we have a Controller for an AbstractPage ? What is an Abstract page? This might be nitpicking, but these kind of namings lead to confusion. Either use class AbstractController or interface PageController.


Every Controller class we create needs to have 2 methods, setup and compile. However, I'm not so sure these are good names, they even smell.

setup: Whats wrong with using __construct()? This was basically created for setup.

compile: It's PhP, why should I compile, is there some kind of .Net integration? Compiling a view, I understand, but a Controller? You lost me. maube run is better.

Especially given the fact that your comment reads:

// run the controller, let it prepare a view and return the rendered code

wouldn't it make more sense to have the following code?

return (new $controllerName())->run()->renderView();

Also note that the word 'run' does not really tell us anything about what is being returned. Making us guess. What are we calling renderView() on? Try to avoid chaining unless they really make sense (Again, mabe it's me, but I just hate 'unknown method on undefined' errors). Use the __construct instead of run()/setup() and we get the following code:

return (new $controllerName())->render()

This doesn't even need comments, saves you a few key strokes #lazyDev


I also see no way of passing arguments to a Controller. Planning on using $_GET there? IF so, here is my sad face :(


Minor remarks

Your Controller has some magic strings:

'\\App\\Controllers\\'

and

'Controller'

maybe you should replace it by some constant or similar. Simply because CONTROLLER_NAMESPACE makes more sense than '\App\Controllers\'. Also make sure that the namespace is only hardcoded in one place. I just don't like hard coded strings.

\$\endgroup\$
  • \$\begingroup\$ First of all - thank you for your reply! I picked up the convention to append "Controller", "Interface" etc. to classes somewhere. But you are right, I'll come up with new names :) I have an AbstractPageController because there is also an AbstractPartialController (for partials like header/footer/...). Should I rethink this completely? Then, "compile" refers to the way my controllers works: They fill the variables with data, then build a view with them and return a response. But the return (new $controllerName())->render() is really a great idea, as are arguments - completely forgot'em! \$\endgroup\$ – Moritz Friedrich Jun 17 '15 at 22:21
  • \$\begingroup\$ And what is the exact difference between a PageController and a PartialController? Seems like a PageController is a spcial case of a PArtialController as in it can include other Controllers? Maybe just have a 'render' method that can easily render child Views. A good place to start is to see how e.g. Twig compiles themplate files to classes. Analyse those classes and see how they handle partials, etc. \$\endgroup\$ – Pinoniq Jun 18 '15 at 10:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.