To register for an Internet.com membership to receive newsletters and white papers, use the Register button ABOVE.
To participate in the message forums BELOW, click here
PHPBuilder.com  
 

 

Go Back   PHPBuilder.com > PHP Help > Code Critique

Code Critique Having someone critique your code is always a great way to hone the skills. Stop in and post your code to see what your peers may have done differently.

Reply
 
Thread Tools Rate Thread Display Modes
Old 01-21-2004, 02:03 PM   #1
tshafer
Programmer Extraordinaire
 
Join Date: Aug 2002
Location: Wilmington, NC
Posts: 67
Personal Template Class

Hey all, I'm writing a custom template for my website, and decided to put up what I have for inspection and optimization!

The file is attached. A typical page would request the template like this:

PHP Code:
<?php

include('/home2/somesite2/secure/class.template.phpx');
$objTemplate = new WebTemplate(array('New', 'Page'), './');

$strSideText = 'This is where the descriptive text for a page goes.
      You see, this is a cool place, a wonderful place; a place that can work wonders
      for the navigation of the site.'
;

$objTemplate->CacheResults(true);
$objTemplate->AddStylesheet('printers.css', 'print');
$objTemplate->AddSidebarText($strSideText);
$objTemplate->RenderHeader();

?>
Any critiques and optimizations of either the class or the access code is very welcome.

---tom
Attached Files
File Type: txt class.template.phpx.txt (9.0 KB, 96 views)
__________________
Tom
Port City Community Church
tshafer is offline   Reply With Quote
Old 01-27-2004, 12:07 PM   #2
lastcraft
Junior Member
 
Join Date: Jan 2004
Location: London
Posts: 3
Re: Personal Template Class

Hi...

Quote:
Originally posted by tshafer
Any critiques and optimizations of either the class or the access code is very welcome.
The first thing that strikes me is that very long constructor. Seems you are setting lot's and lot's of variables. Is there a way of grouping some of these into classes of their own. Small classes are a good thing. They can then each intialise themselves within their own constructors.

There is not yet a lot of abstraction (not unusual for a first cut).

More worrying is the choice of defaults. If a vital value is not set (such as the URL) then you should bomb out as quickly as possible and spectacularily as possible. Using trigger_error() should do the trick nicely. Having something silently work with a misleading default value will lead to bugs that are very hard to find. Set it to Null and do an error check for null on use.

If this thing is general purpose (classes should aspire to this), why is the template filename not taken in the constructor? It would make the client code much more descriptive and easier for an outsider to modify. It would be very be very obvious that the control code applied to a specific view.

Also worrying is the mention of databases in the template engine. What has a template class got to do with a database? You should definitely hack this. Any other solution is going to be better than that.

Apart from the constructor, the rest of the code is clear enough. Nice short descriptive methods. One of teh requirements of this group is that code be commented. I think this is bogus. Code should be easy to read and you use comments to clear up special cases and ambiguity. Most of your comments actually say nothing. I would strip the lot. Frees up time for refactoring the code .

yours, Marcus
lastcraft is offline   Reply With Quote
Old 01-27-2004, 03:50 PM   #3
planetsim
code | beer > sleep
 
Join Date: Sep 2002
Location: aus
Posts: 4,826
Not a bad Templating Class.

However i feel it is too bulky. You are trying to do way to much within the class.

I have a few questions about why do you need the Jscript section, DisplayBodyTag. etc. Im not sure if you are making the template on the fly or if you just adding extra data to an existing template already premade.

Other than those just needing to be cleared up its quite good. Different i must admit.
__________________
Dont be lazy Search
And use the Manual
Webmobo - Open Source News Script | Portfolio / Blog | Against TCPA
planetsim is offline   Reply With Quote
Old 01-28-2004, 01:02 PM   #4
tshafer
Programmer Extraordinaire
 
Join Date: Aug 2002
Location: Wilmington, NC
Posts: 67
Maybe there's a better way to do this, then. I simply need something that I can call that will include header, footer, nav, etc. and be flexible to add code (like CSS declarations or JS declarations) to the header of an HTML document when needed. I'm not sure, since this is my first try at both this kind of program and working with classes.

---tom
__________________
Tom
Port City Community Church
tshafer is offline   Reply With Quote
Reply

Bookmarks


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes Rate This Thread
Rate This Thread:

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is Off
HTML code is Off
Forum Jump


All times are GMT -4. The time now is 09:29 PM.






Acceptable Use Policy

internet.comMediabistrojusttechjobs.comGraphics.com

WebMediaBrands Corporate Info


Advertise | Newsletters | Feedback | Submit News

Legal Notices | Licensing | Permissions | Privacy Policy


Powered by vBulletin® Version 3.7.2
Copyright ©2000 - 2009, Jelsoft Enterprises Ltd.