tags:

views:

1034

answers:

7

I have this marked as PHP but only because I'll be using PHP code to show my problem.

So I have some code like this for the controller:

switch ($page)
{
    case "home":
        require "views/home.php";
        break;
    case "search":
        require "views/search.php";
        break;
}

Obviously there's more pages but this should illustrate my issue. There is a common header, navigation and footer for both of these pages (and for all pages on the site for that matter). Should I be using multiple require statements? My first guess would be:

switch ($page)
{
    case "home":
        require "templates/header.php";
        require "templates/navigation.php";
        require "views/home.php";
        require "templates/footer.php";
        break;
    case "search":
        require "templates/header.php";
        require "templates/navigation.php";
        require "views/search.php";
        require "templates/footer.php";
        break;
}

Some how my gut tells me this isn't correct ...

+4  A: 

The controller should just set up the data for the view and choose which view to display. The view should be responsible for the layout of the page, including shared pages. I like your first sample over the second.

tvanfosson
That doesn't answer my question, I don't think that having the same header/footer/navigation on both files in smart. Especially as it blatantly defies the DRY principle.
Andrew G. Johnson
Not necessarily. ASP.NET MVC offers master pages which allows both separation of concerns (which is what I was talking about) and DRY principles to be maintained. Don't know enough about php frameworks to know if there is one with a similar feature. For me maintaining SOC is more important than DRY
tvanfosson
+1  A: 

Yes, you should have the header, footer, etc. split out.

For the particular example you show, wouldn't this work better?

    require "templates/header.php";
    require "templates/navigation.php";
    require "views/$page.php";
    require "templates/footer.php";

(Where $page is 'home', 'search', etc.)

genehack
A: 

Is there any particular reason why you're not using an MVC framework such as Symfony or CakePHP?

I know this does not answer the question directly, but it might be helpful.

andyuk
Don't forget [Zend](http://framework.zend.com/)
Zoredache
A: 

If you are using straight PHP pages as your templates, you could essentially set a global/session variable to hold the page you want. You would have a "main template" php page which includes the header and footer elements, then calls a include for the $page. Something like this in the controller:

$_SESSION['page'] = sanitize_input($_GET['page']);
require "templates/main.php";

and then in the main.php template file:

require "templates/header.php";
require "templates/navigation.php";
require "views/{$_SESSION['page']}.php";
require "templates/footer.php";
Alarion
A little curious as to why this is being voted down when it directly answers the OP's question. If he doesn't want to use this format, use a template engine or a framework. Any other way will result in repeating code.
Alarion
$_SESSION should not be used here. We're not carrying data over to the next request. Otherwise you're headed in the right direction.
Preston
I was just using that as a simplified example. Storing it in a data structure of some sort so that the included pages can reference it is fine. I don't like carrying over regular "global" variables. *shrug*
Alarion
A: 

Here's a simplified version of how I do templates with my current project, if it's any use:

class Template {
    var $pagename = 'index';

    function __construct() {
        $this->pagename = basename($_SERVER['SCRIPT_NAME'], '.php');
        register_shutdown_function(array($this, 'do_output'));
    }

    function do_output() {
        $this->header();
        $this->display($this->pagename);
        $this->footer();
    }

    function __call($template, array $params) {
        call_user_func(array($this, 'display'), $template, params);
    }

    function display($template, array $params = null) {
        include "templates/$template.php";
    }
}

The idea behind it is that you can write "include 'Template.inc'; new Template;" and it arranges for do_output() to run at the end of the script automatically. There's a few things left out from it like the method used to pass variables to the template.

You've mentioned you're not using PHP, and there's a few PHP-isms in there: register_shutdown_function() makes sure the templates get called before object destructors but after the main script, and the calls to $this->header()/footer() are magic function calls that just do display('header') and display('footer'), they're meant to be overridden.

Of course there's nothing wrong with using a switch like the example you've posted, but you don't need the headers/footers inside every case statement. Something like this would do the same thing:

require "templates/header.php";
require "templates/navigation.php";
switch ($page)
{
    case "home":
        require "views/home.php";
        break;
    case "search":
        require "views/search.php";
        break;
}
require "templates/footer.php";

...or you could replace the switch() with something based on the filename like I've used above, if it works for the way your pages are set up. A switch is the safest way if you plan on doing it through URL parameters though.

Ant P.
A: 

You're repeating code. This is next to never a good idea. To stay close to your initial example, something like this would surely be preferable:

require "templates/header.php";
require "templates/navigation.php";

switch ($page) {
    case "home":
        require "views/home.php";
        break;
    case "search":
        require "views/search.php";
        break;
}

require "templates/footer.php";

It's hard to give more advice without knowing more about your architectural approach. For instance it would be advisable to have this part of the controller, which simply prepares the output, in a very central place and to start output buffering before including the view templates. That way you could store the output in a variable which you might want to process further before returnin its contents in the HTTP response.

A: 

I agree with tvanfosson, and want to explain why and how it relates to MVC.

The problem with your second example is that the controller is exposed to how the view is constructed. In a strict sense, the controller marshals the inputs for the view and passes them to it, and nothing more.

A practical way of thinking of this is if the view shifts depending on application requirements or the inputs themselves. For example, if the view being generated is for a JavaScript pop-up, it might (and probably will) use a different set of headers, footers, CSS, meta, etc. With your second example, all that is exposed to the controller. In your first, it's the view who knows how to generate the view -- which is exactly the point.

To take my example further, imagine that the JavaScript pop-up is redesigned to be a full page view, or is refactored for AJAX (or the pop-up/page/AJAX question is determined by inputs, such as a hidden element in a field). Now you're ripping apart the controller because the view has changed. It's not so much that you've violated MVC, but you shouldn't have bothered with it in the first place.

Jim Nelson