Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've just completed coding my template engine. I'm willing to have a review on how to increase the quality, efficiency, elegance and performance of the code, perhaps by shortening the code, etc. I'm also open for suggestions on other ways of doing this. It would be really appreciated if you point out every tiny bit of things, as I am willing to modify and improve the code accordingly.

Model class:

<?php
class Model {
    public $site_url;
    public $theme;
    public $params;

    public function __construct($global) {
        $this->site_url = $global['site_url'];
        $this->theme    = $global['theme'];
    }
    public function createParam($key, $value) {
        $this->params['{' . $key . '}'] = $value;
    }
    public function replaceParams($content) {
        foreach($params as $key => $value) {
            str_replace($key, $value, $content);
        }
        return $content;
    }
}

View class:

<?php
class View {
    private $model;
    private $controller;

    public function __construct($model, $controller) {
        $this->model      = $model;
        $this->controller = $controller;
    }
    public function handlePageLoad($page_name) {
        $this->path = $model->site_url . "/app/tpl/skins/" . $model->theme . "/" . $page_name . ".php";
        if($controller->pageExists($this->path) === true) {
            $this->content = $controller->createPage($this->path);
            return $this->content;
        } else {
            $this->newPath = $model->site_url . "/app/tpl/skins/" . $model->theme . "/404.php";
            $this->content = $controller->createPage($this->path);
            header('Location: ' . $model->site_url . "/index.php?page=404");
            return $this->content;
        }
    }
}

Controller class:

<?php
class Controller {
    private $model;

    public function __construct($model) {
        $this->model = $model;
    }
    public function pageExists($path) {
        if(file_exists($path)) {
            return true;
        } else {
            return false;
        }
    }

    public function createPage($path) {
        $this->content = $model->replaceParams(file_get_contents($path));
        return $this->content;
    }
}

Global.PHP

<?php
function __autoload($class_name) {
    include "classes/class." . $class_name . ".php";
}

$global = array(
    'site_url' = 'http://localhost/Projects/HassanCMS',
    'theme'    = 'v1'
);

$model      = new Model($global);
$controller = new Controller($model);
$view       = new View($model, $controller);

$model->createParam('site_title', 'HassanTech');
share|improve this question
    
OK, let's start with, the Model class is completely broken. PHP has no implicit $this, so none of the code there gets or sets anything on the object. –  cHao Jul 19 at 17:47
    
What do you mean? @cHao –  Hassan Althaf Jul 19 at 17:47
    
Oh wait yer, changed. –  Hassan Althaf Jul 19 at 17:48
    
I mean that in PHP, you can't say $property -- you have to say $this->property. So all the variables you ever set in that code, are local variables -- the code has no effect, and will probably emit notices. –  cHao Jul 19 at 17:48
    
@cHao I know, I confused it with a parameter. –  Hassan Althaf Jul 19 at 17:49

2 Answers 2

up vote 5 down vote accepted

There are at least two places in your code where it looks like you wanted to use an object's attributes, but you say $attribute rather than $this->attribute. PHP requires $this-> before property accesses. If it's not there, PHP assumes there's a local variable. Setting stuff that way will have no effect, and trying to read it will trigger a notice about undefined variables. In fact, i'd be rather surprised if the code works at all.

But with the brokenness set aside, it looks like your three classes are quite confused about their jobs. Let's review what each part of an MVC triad is supposed to do:

  • The "view" basically handles all the rendering.
  • The "model" represents the business objects, and should be the meat of the application.
  • The "controller" coordinates stuff, routing stuff to the right place, etc.

Now, let's take a look at your classes.

  • View tries to directly manage requests, doing the routing that the controller (or a router script) should be doing.
  • Model is basically filling in a template. That's part of the rendering, so that's the view's job.
  • Controller barely does anything. It just tells the model to replace params, and determines whether a template exists. It shouldn't be doing either one; that's the view's job.

As for why all that is bad: What happens if you want to use a template library like Twig or Smarty? If your concerns were properly separated (ie: if everything were in the right place), you'd only have to change the View class. Right now, though -- since rendering stuff is spread throughout the app -- you have to change everything. This is why MVC was invented: to keep each part separate, so you can change one part without breaking the others.

Oh...and as i'm looking at it...it doesn't look like these scripts actually do anything. $view->handlePageLoad never gets called.


Other possible issues:

  • You're redirecting to a 404 page...and that page is probably not returning a 404 status code to the browser. If Google ever finds a broken link to your site, it'd think that was the content...and your error page would end up indexed.

    Rather than redirecting, you could simply say header('HTTP/1.1 404 Not Found');, and print out the contents of your 404 page.

  • __autoload is discouraged and may be removed in future versions of PHP. Instead, use spl_autoload_register to register an autoloader (which can be named whatever you like). Then you can have more than one autoloader. That will be very important later, when your site grows and you want to include other people's libraries, like Composer packages, which have their own autoloaders.

share|improve this answer
    
How can I send a 404 error code. Can you also simplify your language? I am finding it difficult to understand. I suck at english. –  Hassan Althaf Jul 19 at 18:48
    
I've updated the code as you have suggested me. Please check the updated code here; codereview.stackexchange.com/questions/57481/… –  Hassan Althaf Jul 19 at 22:21

MVC

The handlePageLoad method in View is doing a bit too much:

  • It does checking if a page exists or not
  • It knows too much about how the site works, the paths, the 404 page

I think it would be better if the View was dumber: each page of your site should have a corresponding render_X method, with the necessary parameters to render it. It should be the Controller to check if the requested link corresponds to a page that can be rendered (= routing).

Style

This method can be simplified:

public function pageExists($path) {
    if(file_exists($path)) {
        return true;
    } else {
        return false;
    }
}

like this:

public function pageExists($path) {
    return file_exists($path);
}

Actually, if you refactor the code to move the routing logic into the Controller, then the method will be pointless, as the Controller can simply call file_exists directly, and the other classes shouldn't need to know about a "pageExists" method.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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