Print rather than Echo
This one's more for consistancy, (we've used echo everywhere before, it's it's one character shorter ;)
Grepping for echo in a project, is also quite a handy test for any kind of data injection, since most output is our projects is done via template engines. Grepping for :h and echo will find most potential holes. (althoug grepping for print_r / var_dump is also a good idea..)
Exiting on XMLHttpRequest calls.
Most of the XUL based code, and alot of the new HTML/XUL hybrid code I specifiy uses XMLHttpRequest calls to send update/delete/add requests,
Rather than using standard HTML POST's on the form, we build a POST request manually(well with a simple library), and send it via XMLHttpRequest, this we we can test the return, and if there was a problem, we dont have to re-render the form, or re-open a popup.
Unfortunatly since alot of the more recent project are confidential, there have not been many examples I've been able to show the developers of how to do this.
In standard FlexyFramework operation, you would run get/post, and expect it either to set up all the variable for the template to be rendered, or redirect you to another page. So for a XMLHttpRequest that was expecting 'OK' or an error message, the last stage of the FlexyFramework does not fit in with the desired behaviour.
One of the changes that had to be made, was removing some logic that the developers had put in. - They came up with the idea of sending ?def=html, def=txt, def=xml, def=xul etc. to determine the return type, and then overloaded the output method to either die there, and then added header def's to every page.
The correct way to handle this was to use the extension of the $page->template variable (eg. fred.html=>html, fred.xul => xul) to determine type for html & xul,
Then for text and xml output use headers() + exit; at the end of the get/post requests for text/xml output. (although txt does not really need a header)
function get()
{
do stuff();
if (some_error_occurs()) {
echo "The error message";
exit;
}
echo "OK";
exit;
}
// or with XML:
function get()
{
do stuff();
if (some_error_occurs()) {
echo "The error message";
exit;
}
header('Content-type: text/xml');
echo $xml;
exit;
}
Dont do the page processing in the output method
Since every controller class extends the projects base class, and the base class usually implements get($str), and tests if $str is not empty -> hence 404 - not found. A page implementing the input/output in the output() method will accidentaly call the get() method and return 404 - causing XMLHttpRequest caller to think there is a problem. - Basically they should just have used the get()/post() methods.
Avoiding shortcut's in Javascript
Pretty much applies to PHP as well, by creating a load of shortcuts, you make the code very difficult to follow, being a little verbose may take a few extra characters, but will save the debugging effort later no end.
This is one of those things, that once you get started, you usually dont know when to stop, resulting in build a huge javascript library that is sneakly included on every page. - making you umm and ahh, every time you see a strange looking function call.
Using Libraries that where not specified or non-PEAR ones.
While PEAR is not perfect, fixing things in PEAR is feasible, alot of the code is very well tested andusually kept update with latest PHP versions. Hence and warnings and errors have been fixed.
In both projects external libraries appeared as dependancies, sajax, and HTML2PDF. For the first, I consider that HTML_Javascript should provide all the necessary features that where being utilized from sajax (we where only outputing json data!), and since I know HTML_Javascript inside out, fixing or working around issues would be easier in the long run than introducing a rarely used library into a projects.
For the second we ran into huge compatibility problems with newer PHP4 and PHP5 versions with HTML2PDF, and although I had agreed to use it when we disucussed it during development. It proved to be more trouble than it was worth, especially as the perl html2ps worked perfectly well.
Using oncommand, rather than commandset/command
In XUL, you can group commands into command sets, and use command="xxx" rather than oncommand="callsomething()". however, this just makes mapping an action to some code involve an unneccessary step of deduction, that rarely adds much value. I suspect oncommand is more suited to XUL based xulrunner app's rather than XUL apps that work in a similar way to HTML applications.
dirname(__FILE__) or cwd()
In general I regard cwd as a bit hit or miss - sometimes it worked, and sometimes it doesnt - and I dont have time to read the manual to work out why, when a perfecty good alternative exists - dirname(__FILE__) gives predictable results, so that's the way the rule get's laid down. Existing proven methods over doing things differently...
Cloneing dataobject arrays over creating simple structs.
One of the pieces of code that was used to store data prior to rendering looked a bit like
while($do->fetch()) {
$ret[] = (object) array('user'=>$do->user, 'id' => $do->id));
}
While it's a little less efficient memory wise, the reality was that the whole data from dataobject would probably be needed later (as things like emails that use the data where only drafts, and would probably require more details)
so the code should have said
while($do->fetch()) {
$ret[] = clone($do);
}
dont hard code options into the controller class
unfortunaly, one of the things that was added to the base controller class of the project early on, and has been difficult to remove is this little gem
function __construct() {
$opts= PEAR::getStaticProperty('MyProject','options');
foreach($opts as $k=>$v) {
$this->$k = $v;
}
}
This has the horrible consequence of hiding a potentail variable mis-used (eg. size is an option and we use $this->size later for something else). The correct way to do something like this would be.
function loadProjectOptions() {
$opts= PEAR::getStaticProperty('MyProject','options');
$this->projectOptions = $opts;
}
And calling that from any get() method on pages that needed it, in a similar ilk, not calling that from the template is also frowned apon, (eg. {loadProjectOptions()} ..... ) as if the method did database calls, you could end up with some strange results.
Well, that's all for now.. Thankfully the code in general on both projects is pretty good, so maintaining, and growing the code to meet future needs, has like older projects should save me considerable time, when that infamous request comes in 12 months later... 'can you change this?'...