1
\$\begingroup\$

I am trying to create a web app (CMS) as part of my research project, and I use the code below to load modules into a page. Please review this code.

public function LoadModule($module,$position){

    if(file_exists('modules/'.$module.'.tpl'))
    {
        //todo: might have to get a better thing for this !
        $mod = 'modules/'.$module.'.tpl';
        ob_start();
        include($mod);
        $mod = ob_get_clean();

        $this->html = str_replace('__{'.$position.'}__', $mod ,$this->html); 
    }
    else{
        if(function_exists('LOGIT')){

            LOGIT('$module - module not found.');
        }
        else {
            $this->html = str_replace('__{'.$position.'}__', "$module Modlule not found" ,$this->html);
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

I don't know if it's a paste-issue, but you should intent your code ;)

You are building the file-path twice. You should move that line:

$mod = 'modules/'.$module.'.tpl';

To the begining of the function (and give it a better name):

public function LoadModule($module,$position)
{

    $absTemplatePath = 'modules/'.$module.'.tpl';
    if(file_exists($absTemplatePath ))
    {

That Line:

LOGIT('$module - module not found.');

should be (else $module wouldn't get replaced):

LOGIT("$module - module not found");

If the template file does not include php-code you should use file_get_contents instead of ob_start, include, ob_get_clean (performance-wise). If you do have php-code in your templates, you might have a look at zend-framework's Zend_View.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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