Proper way of using the MVC concept - update

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:

site_url = $global['site_url'];
        $this->theme    = $global['theme'];
    }
}

View:

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:

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:

createParam('site_title', 'HassanTech');

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?

StackExchange Code Review Q#57481, answer score: 7