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.
'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.
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.
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().
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.
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.
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.