Template - Page rendering class - Updated

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
dayyanb
Forum Commoner
Posts: 46
Joined: Wed Jan 23, 2008 12:34 am

Template - Page rendering class - Updated

Post by dayyanb »

Is this a proper way to do it?

Code: Select all

<?php
    class Page {
        private $pageVars = array();        
        private $serverroot;
        
        private $siteInfoLocation;
        private $templateLocation;
        
        public function __construct($serverroot) {
            $this->serverroot = $serverroot;
            
            $this->siteInfoLocation = $this->serverroot . 'siteinfo/';
            $this->templateLocation = $this->serverroot . 'templates/';
            
            $this->pageVars['body'] = '';
            $this->pageVars['name'] = '';
            $this->pageVars['title'] = '';
            $this->pageVars['template'] = 'default';
            $this->pageVars['navbar'] = array();
            $this->pageVars['keywords'] = $this->loadfile('keywords');
            $this->pageVars['description'] = $this->loadfile('description');
            $this->pageVars['footer'] = $this->loadfile('footer');
        }
        
        public function __set($var, $value) {
            //Return false if they are trying to set a nonexistant variable.
            if(!isset($this->pageVars[$var])) {
                return false;
            }
            //Return false if they are trying to set a variable to the wrong type.
            if((is_string($this->pageVars[$var]) != is_string($value)) && 
            (is_array($this->pageVars[$var]) != is_array($value))) {
                return false;
            }
            $this->pageVars[$var] = $value;
        }
        
        private function removeSlashes($var) {
            $var = preg_replace('/\\//', ':', $var);
            return $var;
        }
        
        private function loadFile($file) {
            $file = $this->removeSlashes($file);
            $filepath = $this->siteInfoLocation . $file . '.txt';
            $filesize = filesize($filepath);
            if($filesize > 0) {
                $data = file_get_contents($filepath);
            } else {
                $data = '';
            }
            return $data;
        }
        
        public function renderPage() {
            $pageVars = $this->pageVars;
            $pageVars['template'] = $this->removeSlashes($pageVars['template']);
            $templatefile = $this->templateLocation . $pageVars['template'] . '.php';
            require_once($templatefile);
        }
    }
?>
EDIT: updated code again.
Last edited by dayyanb on Wed Jan 30, 2008 9:18 pm, edited 5 times in total.
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Re: Template - Page rendering class

Post by pickle »

'Proper' is very subjective - there's an infinite number of ways to do anything in code ;) There are a few things I would do to improve what you've got there:
  • Don't typecast in your __construct() method. There's no need to.
  • Use file_get_contents() in your loadFile() function.
  • If you're using PHP5, don't create a new function setVar(), overwrite the magic method __set() (which essentially does the same thing, but invoking it is cleaner).
  • In your fixVars() function, you probably want to check $this->pageVars['title'] rather than $this->title.
  • You should set the directory you use in $filepath in loadFile() and $templatefile in requireTemplate(), in the constructor() - save yourself some duplication.
  • You should probably play it safe and put some checks in requireTemplate() and loadFile() to make sure the supposed filename isn't trying to navigate through your directory structure.
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
dayyanb
Forum Commoner
Posts: 46
Joined: Wed Jan 23, 2008 12:34 am

Re: Template - Page rendering class

Post by dayyanb »

pickle wrote:'Proper' is very subjective - there's an infinite number of ways to do anything in code ;)
a proper.
In your fixVars() function, you probably want to check $this->pageVars['title'] rather than $this->title.
:oops:


I'll fix those things and repost a new version, thanks a lot for the advice.
Last edited by dayyanb on Mon Jan 28, 2008 5:22 pm, edited 1 time in total.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Template - Page rendering class

Post by Christopher »

Proper would be to pass the path to the constructor and not have a global. And I am no sure what fixVar() is for ... it seems like a kludge to me. Plus, it causes requireTemplate() to be separate from renderPage().
(#10850)
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Template - Page rendering class

Post by Mordred »

The PHP Manual wrote:Never use gettype() to test for a certain type, since the returned string may be subject to change in a future version. In addition, it is slow too, as it involves string comparison.

Instead, use the is_* functions.
dayyanb
Forum Commoner
Posts: 46
Joined: Wed Jan 23, 2008 12:34 am

Re: Template - Page rendering class

Post by dayyanb »

arborint wrote:Proper would be to pass the path to the constructor and not have a global. And I am no sure what fixVar() is for ... it seems like a kludge to me. Plus, it causes requireTemplate() to be separate from renderPage().
I can pass the path instead. I want to make the title equal to the name if the title is not set, but I guess this really belongs in the template itself.

Thanks everyone.
dayyanb
Forum Commoner
Posts: 46
Joined: Wed Jan 23, 2008 12:34 am

Re: Template - Page rendering class

Post by dayyanb »

Mordred wrote:
The PHP Manual wrote:Never use gettype() to test for a certain type, since the returned string may be subject to change in a future version. In addition, it is slow too, as it involves string comparison.

Instead, use the is_* functions.
I was comparing a gettype to another gettype so it wasn't really applicable under these circumstances (unless you count the "NULL"). However I know the variables would only be an array or string so I will just check for those.
Scrumpy.Gums
Forum Commoner
Posts: 71
Joined: Thu Aug 30, 2007 2:57 pm
Location: Bristol, UK

Re: Template - Page rendering class - Updated

Post by Scrumpy.Gums »

dayyanb wrote:

Code: Select all

private function removeSlashes($var) {
$var = preg_replace('/\\//', ':', $var);
return $var;
}
Why not use str_replace here instead?
dayyanb
Forum Commoner
Posts: 46
Joined: Wed Jan 23, 2008 12:34 am

Re: Template - Page rendering class - Updated

Post by dayyanb »

Scrumpy.Gums wrote:
dayyanb wrote:

Code: Select all

private function removeSlashes($var) {
$var = preg_replace('/\\//', ':', $var);
return $var;
}
Why not use str_replace here instead?
Because I should have. Thanks.
Post Reply