2
\$\begingroup\$

I wanted to know how I can improve it. I mainly want to look at speed and performance, but if there are any improvements please give a suggestion.

<?php
defined("SECURE") or exit('Please define SECURE keyword to continue.');
class template
{
    private $templateParams;

    public function __construct()
    {
        $templateParams = array();
    }

    public function loadTemplate($configClass)
    {
        $this->organiseParams($configClass);
        $this->displayContent($configClass);
    }

    private function organiseParams($configClass)
    {
        $values = $configClass->getAll();

        while(list($title, $value) = each($values))
            $this->addParameter('[config ' . $title . ']', $value);
    }

    private function displayContent($configClass)
    {
        if (strlen($_GET['page']) >= 1)
        {
            ob_start();

            if (file_exists(ROOT . $configClass->getConfig("style.folder") . '/' . $configClass->getConfig("style.name") . '/' . $_GET['page'] . '/public_html/content.php'))
                include ROOT . $configClass->getConfig("style.folder") . '/' . $configClass->getConfig("style.name") . '/' . $_GET['page'] . '/public_html/content.php';
            else
                include ROOT . $configClass->getConfig("style.folder") . '/' . $configClass->getConfig("style.name") . '/error/public_html/content.php';

            $htmlOutput = ob_get_contents();
            ob_end_clean();

            while (list($paramTitle, $paramValue) = each($this->templateParams)) {
                $htmlOutput = str_replace($paramTitle, $paramValue, $htmlOutput);
            }

            echo $htmlOutput;
        }
        else
        {
            header("location: " . $configClass->getConfig("website.url") . "/index");
        }
    }

    private function addParameter($title, $value)
    {
        $this->templateParams[$title] = $value;
    }
}
?>
\$\endgroup\$
0

2 Answers 2

1
\$\begingroup\$

Your first concern is performance, so let's start there. The way you're including and processing your content is going to slow you down. Because you're buffering it, then putting it to a variable, then doing a search replace, you're going to end up working through the content several times before you get your end result. If the content is small then don't even worry about it, but if you have embedded base64 images or just tons of content, then you're going to want a different approach.

From a code point a view I think you're on the right track with your object design. It's always good to break things out to make them easier to maintain. One thing you want to consider though is to take a more MVC style approach by separating processing from the visuals. This makes code easier to read and much easier to maintain. The long story short of this point is to change the echo of the output to a return so that your calling (visual) code can output it. It sounds minor but as your application becomes more complex, it can be a headache saver.

Back to your first point, how to improve getting of the content. My approach would be to make the included content an object file. The file could then include a get method of sorts that returned the html in a string with your titles and parameters already in place. That would allow you to have your content file be built just like a concatenated string (saving the find and replace work). Pass your values as parameters and you're good to go.

Here's how I would do this. I didn't include code for the content class as the code in this Template class should be enough to go on. Note that based on my example, all include files should have the class ContentObj with the method getOutput. I simplified your include path code to make things much easier to read. I also added an exit after your header redirect as there's no need for further processing (usually) after a redirect.

<?php
defined("SECURE") or exit('Please define SECURE keyword to continue.');
class Template
{
    private $configClass;
    private $templateParams;

    public function Template($configClass)
    {
        $this->configClass = $configClass;
        $this->templateParams = array();
    }

    public function loadTemplate()
    {
        $this->organiseParams();
        $this->displayContent();
    }

    private function organiseParams()
    {
        $values = $this->configClass->getAll();

        while(list($title, $value) = each($values))
            $this->addParameter('[config ' . $title . ']', $value);
    }

    private function displayContent()
    {
        if (strlen($_GET['page']) >= 1)
        {
            $contentPath = ROOT . $this->configClass->getConfig("style.folder") . '/' . $this->configClass->getConfig("style.name");

            if (file_exists($contentPath . '/' . $_GET['page'] . '/public_html/content.php'))
                include $contentPath . '/' . $_GET['page'] . '/public_html/content.php';
            else
                include $contentPath . '/error/public_html/content.php';

            $contentObj = new ContentObj($paramTitle, $paramValue, $htmlOutput);
            $htmlOutput = $contentObj->getOutput($this->templateParams);

            return $htmlOutput;
        }
        else
        {
            header("location: " . $configClass->getConfig("website.url") . "/index");
            exit;
        }
    }

    private function addParameter($title, $value)
    {
        $this->templateParams[$title] = $value;
    }
}
?>
\$\endgroup\$
-2
\$\begingroup\$

First at all, I don't think it's good oop, you can rewrite this class in one function, read some articles about SOLID principles. Try to KISS (Keep It Simple Stupid!) and good luck, you should improve your skills!

Use return instead of echo

It's better to use return, cause you can decide what to do with your output. Echoing things just force us to show it, but what if we want just save it to file??

Use ternary (shortcut that damn ifs!)

You can make that ifs more readable by using ternary operators.

$path = (file_exists($folder . '/' . $_GET['page'] . '/public_html/content.php')
              ? '/' . $_GET['page'] . '/public_html/content.php'
              : '/error/public_html/content.php';

Use objects when you need to

You can rewrite you class as function. Look at this piece of code:

<?php

function loadTemplate(ConfigClass $config)
{
    $params = array();

     foreach ($config->getAll() as $key => $value) {
         $params['[config ' . $key . ']'] = $value;
    } 

    if (strlen($_GET['page']) >= 1)
    {
        ob_start();

        $folder = ROOT . $config->getConfig("style.folder") . '/' . $config->getConfig("style.name");

        $path = (file_exists($folder . '/' . $_GET['page'] . '/public_html/content.php')
              ? '/' . $_GET['page'] . '/public_html/content.php'
              : '/error/public_html/content.php';

        require $folder . $path;

        $htmlOutput = ob_get_contents();

        ob_end_clean();

        foreach ($params as $key => $value) {
            $htmlOutput = str_replace($key, $value, $htmlOutput);
        }

        return $htmlOutput;
    }
    else
    {
        header("location: " . $config->getConfig("website.url") . "/index");
    }
}

It does the same thing. Think about it. Good luck!

\$\endgroup\$
1
  • 2
    \$\begingroup\$ You didn't review his code at all, you just told him to get good. Write an improved oop version then \$\endgroup\$ Feb 21, 2016 at 12:58

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.