6
\$\begingroup\$

Original question:

Template engine

I updated my files as instructed, and now I require a review. Please point out the most minute things out to me for me to have better code. Once again, efficiency, elegance, and performance is what I need. Also, please point out places where better code can be used (for places like loading a template, I need to find a better way to load a template).

Model:

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

    public function __construct($global) {
        $this->site_url = $global['site_url'];
        $this->theme    = $global['theme'];
    }
}

View:

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

    public function __construct($model, $controller) {
        $this->model      = $model;
        $this->controller = $controller;
    }
    public function createPage($path) {
        $this->content = $model->replaceParams(file_get_contents($path));
        return $this->content;
    }
    public function createParam($key, $value) {
        $model->params['{' . $key . '}'] = $value;
    }
    public function replaceParams($content) {
        foreach($model->params as $key => $value) {
            str_replace($key, $value, $content);
        }
        return $content;
    }
    public function pageExists($path) {
        return file_exists($path);
    }
}

Controller:

<?php
class Controller {
    private $model;

    public function __construct($model) {
        $this->model = $model;
    }
    public function handlePageLoad($page_name) {
        $this->path = $model->site_url . "/app/tpl/skins/" . $model->theme . "/" . $page_name . ".php";
        if($view->pageExists($this->path) === true) {
            $this->content = $view->createPage($this->path);
            return $this->content;
        } else {
            header('HTTP/1.1 404 Not Found');
            return false;
        }
    }

}

Global:

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

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

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

$view->createParam('site_title', 'HassanTech');
\$\endgroup\$
0

1 Answer 1

7
\$\begingroup\$

I'll skip MVC for now, as I see a whole review for formatting and the such. Perhaps someone else could break down the pattern for you. Right now, let's focus on each individual class.

Model

  • public!? What's your logic behind that!? Are we programming object-oriented code here? Part of OOP is designing with encapsulation in mind. Encapsulation is using those getters and setters. So in your case, we might find:

    protected $site_url;
    
    public function getSiteURL() {
        return $this->site_url;
    }
    

    Here are some benefits of doing this:

    By keeping data private and providing public well-defined service methods the role of the object becomes clear to other objects. This increases usability.

    Source

    The shielding of the data means that should the format of the private data need to change for some reason the objects invoking methods on the object whose data has changed will not need to change the format they send their messages in.

    Source

    Encapsulation is used to hide the values or state of a structured data object inside a class, preventing unauthorized parties' direct access to them.

    Source

  • You should type hint $global with array.

  • In general, it seems strange to be passing this type of configuration data as an array in the constructor. If you had a configuration class, then you could pass that, and then the model would have to worry less about the information it's handling. It will only get the information it wants, yet it's expandable so you can later add more information.

View

  • You could improve the constructor to something like:

    __construct(Model $model, Controller $controller)
    

    This way you know exactly what's being thrown at you.

  • You raised the scope of $this->content, which is unnecessary. Keep it local.
  • '{' . $key . '}': it would make more sense to do this as you replace, not store.
  • $key and $value aren't very meaningful names.
  • For both createParam and replaceParams, don't be afraid to type out the whole word. It helps no one to shorten it like you have.
  • pageExists has no business living in the view.
  • And probably the biggest issue I see is the lack of support. Right now, this class replaces static values on static pages. What if we need a conditional on the view? ("If the user's logged in, show this. If not, show that") Or maybe a you want to show all the blog posts you stored, well you'll need to build the HTML in your PHP, and then send it all as one parameter! Think about what you can do to support these types of edge cases.

Controller

  • It's not uncommon to see child controllers, so private may want to be protected for $model.
  • Type hint Model in the constructor.
  • Any type of "handler" or "manager" is a bad sign. So either handlePageLoad needs a new name, or the contents need to be broken down. Let's have a look...
  • Well, it looks like the method "handles" to much! Perhaps pageExists should be a controller function, and that way you can work in the path.
  • $this->path should be local to the method. You're reducing the class scope's clarity by not having proper local variables.
  • Normally there's a space after ifs.
  • file_exists returns a boolean only, so the type comparison is overkill. A simple == would suffice.

Global

  • classes is a horrible directory name. I highly suggest you change that. Perhaps break apart the classes by responsibility.
  • Here's anything fuzzy variable name, $global.
  • I don't see the connection between 'theme' and 'v1'. Is "theme" not the best word?
\$\endgroup\$
5
  • \$\begingroup\$ well worded. and concise with examples and formatting. \$\endgroup\$
    – azngunit81
    Commented Jul 23, 2014 at 4:05
  • \$\begingroup\$ Thanks a lot. Can you give me some example code of creating HTML in PHP? Because, I cannot plan well on loading the content. \$\endgroup\$ Commented Jul 23, 2014 at 9:11
  • \$\begingroup\$ Well if you have logic associated with every page, then maybe every page deserves a class to handle that page. PHP is a templating language, however I'll agree that even it isn't the best at it. I suggest you take a look at how other PHP templating engines work, perhaps Smarty or Twig. \$\endgroup\$
    – Alex L
    Commented Jul 24, 2014 at 4:12
  • \$\begingroup\$ Ok, @AlexL I want to make this code as light as possible, as well as flexible. So I just need to have those templates ready on the themes folder, and the website acts upon those templates. So I think a class would not be cool? \$\endgroup\$ Commented Jul 24, 2014 at 6:45
  • \$\begingroup\$ Correct about the templates and how those will work. I'm not sure I understand what you mean by that last sentence though. \$\endgroup\$
    – Alex L
    Commented Jul 24, 2014 at 7:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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