This post originated from an RSS feed registered with PHP Buzz
by Alan Knowles.
Original Post: Code Reviewing.
Feed Title: Smoking toooo much PHP
Feed URL: http://www.akbkhome.com/blog.php/RSS.xml
Feed Description: More than just a blog :)
[** not proof read yet ;) - so send corrections not complaints ;) **] As I scaled up my freelancing work, I ended up going from a one-man coding band into a team situation. This introduced a few years ago, a slightly different way of working.
Phase I
Specification writing, normally involving doing Inkscape interface designs, along with a huge gnumeric code specification, detailing the database design, along with explaining all classes and templates need for a project.
Phase II
Outsourcing, either to third parties, or to team members within the office I work with. Including a roughly 50% complete code review.
Phase III
The oh sh*t, it's getting urgent, we better fix the delivered code and make it work well enough for the client to start testing
Phase IV
The what seems like never ending bug finding and fixing stage.
Two of the larger projects I have at present are in Phase II & III. One is being forced to go live, the other is halfway through development. Both being coded by different teams, however, what is interesting is that the issues that come up in the code reviews both during and after the work had been done are pretty similar.
The Specification stage, perhaps the most labourious and sometimes mind numbing if it gets large, is always interesting to look back at, and the more I do it, in hindsight, It tends to make you realize, there is no such thing as a perfect specification. It always ends up as a best guess, based on what you think is needed and what you think the end user probably wants.
Dealing with end users is always fun, thankfully, this process tends to deal with the end user in two seperate phases - The Specification and the Bug fixing. (Although what normally happens is that after the bug fixing there is another process called changes - which gets dealt with almost as a new project)
What has interested me alot in the last few weeks has been to see how far some parts of the specification I wrote and planned had to be changed dramatically. Usually as you realize that however much thought went into the concept, the actually flow in usability never quite live up to expectations. Alot of the time things are culled on mass during Phase III, to ensure that a deliverable is met, and payment can be demanded (or at least begged for)
Anyway, what I really want to go into is some of the interesting code that comes out of the code review half way through the project, and the complete overhall that has to be done as you push a project live.
Coding Standards
Thankfully, I have managed to cobble together enough coding standard to try and keep the projects on track. Without them, Stages III and IV would become unmanagable, as tracing exactly how code was implemented so that major changes, refactoring would become completly impossible. However, it seems that however clear the standards are, developers have a nack of not following them, either out of habit or just the pressure they are under to deliver as well. - So a certian part of the middle point code review ends up just reiterating where they have missed those elements. Common mistakes include
Not following the 'no if without braces rules'
not cloning dataobjects, as we develop in both PHP4 and test in PHP5
creating javascript libraries with functions in them.
creating PHP includes with functions in them!
Not making the most of the automated url rewriting that FlexyFramework and HTML_Template_Flexy do for CSS, javascript and images.
Using DataObjects' escape() method rather than mysql_escape methods.
Indentation in javascript files.
using error and notice flags, so the text of the message is on the templates, rather than in the PHP code.
dont implement magic constructors on the base controller...
dont overlay config var's into the controller as raw properties..
dont use $do->whereAdd('somevar=1') when $do->somevar = 1 will do..
dont stick the ?> at the end of the file ;)
Never hard code email subjects etc. in code, use templates.
Dont add too many config options - especially file locations.
The code also is a great opportunity to update and rewrite some of the standards so that things like this should not happen any more.
using 'print' rather than 'echo'
for xmlHttpRequest called code, using 'exit' in the get/post methods, rather than trying to get clever with content-type, and modifying the output method.
dont do the page processing in the output method()
stopping the use of javascript shortcuts - ie:
function ID(n) { return document.getElementById(n); }
using libraries that where not specified.. or using non-PEAR libraries when PEAR ones are available...
using oncommand, rather than commandset/command in XUL
using dirname(__FILE__), rather than cwd for reading data files.
never creating new object/array's when cloning dataobjects will work.
never work with session variables from classes outside the one you are in.
always implement a 404 return for unknown get's on the main page.
never pass get/post requests back up to the main page using parent::get()...
dont hard code options into controller classes (eg. currencies or statuses etc.) - put them in the DataObjects, so other pages can get the same list.
Try not to load stuff from the database withing the template - unfortunatly the best way is to add a common call to all the page controllers get/post methods.. - a few more copy/paste code, but alot more clarity..
Always use the internal redirectors to send notices back to the next page. (eg. HTML_FlexyFramework::run('',array('notice'=>array('user_updated_ok'=>true)));
kill that nesting, return or exit from conditions early, eg. negative test, and return, rather than positive test, and if/else.. nested many levels.
be consistant on dataobject methods, in general, dont do updates to a dataobject on a call, unless the method hints' that it might do it... -> eg. $do->approve() or $do->approveAndUpdate(); ...
dont short if on auto boolean casts: $obj->is_verified = Project::isAuth() ? 0 : 1 .. either just cast it (int) , or let DataObjects worry about it...
Creating an instance of an object to call a method that 'could' be static, is not a bad thing, MyProject_DataObjects_Sometable::callaMethod() or $x= DB_DataObject::factory('Sometable'); $x->callaMethod();
If you need to create temporary or semi temporary files, use the Template engines's temp directory, rather than trying to create them in the Code source somewhere (so that changing file permissions on svn up/checkout is not needed) - We can always set up session.save_path to somewhere let temporary in the bootstrapper..
use the bootstrap to define the default options, rather than the ConfigData/default.ini files (an older FlexyFramework idea)
assume that nothing get's deleted from databases (eg. it always get's flagged), so dont write code that actually deletes it, and always check the deleting flag when searching...
Everything extends the base class, even code run via cron jobs.
And sometimes, you realize that the rules you set down may not be totally the best idea... In one case I advocated using $do->setFrom() rather than seeing a big list of map's between $POST[] / or something else and a dataobject. However, in some cases either building the object slowly and clearly or listing what you send to setFrom() makes the code considerably clearer.
In more detail
In the extended entry I'll look a bit closer at some of these little deviations from the ideal...
Previous Topic
