5 points for a self PHP code review

Reviewing code is like checking your own copy before handing in an exam : last exam I took was driving test (incredible, I know), and I passed by 0 the 100 questions quiz. I do remember changing three or four answers while reviewing the answers, and even as I don’t know for sure about it, I have the feeling this saved the day.

Reviewing the code have the same effect, anytime I find something that ‘was not supposed to be there’. Luckily, this happens rarely, but when it happens, it feels much better about the final version of the code. Of course, there is also the unspoken relief of keeping my ego unbruised by a typo (and that’s why I won’t give examples here).

There are 5 types of errors I like to check when I’m reviewing my code.

• Dead code : this is code that can’t be reached. The two common situations I find are multiple returns in a sequence, and constants conditions in an if.
  if (1 || $o->checkForStatus()) {
return $o->getId() ;
return $o->getSize() ;
}
  The above code has it all. The second return is often some temporary code, which was forgotten. The constant condition is a potential debug line. Both are usually easy to remove.
• Immature code : immature code happens when learning how to use a new framework, library or tool. The first attempts are based on documentation examples, and their aim is to makes things works. Once it’s working, it is worth cleaning, trimming or changing with another function learned along the way. Now, this is experience talking !
  
$sqlite = new Sqlite('db.sqlite') ;
$query = " SELECT age FROM customer WHERE id = '$id' " ;
$res = $sqlite->query($query) ;
$row = $res->fetchArray() ;
$age = $row[0] ;

  This may be simplified with :
  
$sqlite = new Sqlite('db.sqlite') ;
$query = "SELECT age FROM customer WHERE id = '$id' " ;
$age = $sqlite->querySingle($query) ;
• Debug code : similar to the reasons that lead to Dead Code, some instructions are introduced in the code to report punctual status. This means var_dump and print_r, but it may also be extra print or echo, or even messages being logged. Depending on the debugging sessions, those messages may be very precise, and not worth keeping in production.
• Left for later : some structures may be introduced in the code for later use. This is the case for classes, interfaces that are derived from another, but won’t get any special code until the class can show some specific behaviour. As a first version, the default parent class is used. During review, I like to make sure those structures are not missing anything specific.
  try/catch/finally or if (xxx_error()) { /* RFU*/ } are both good places to check if anything was forgotten. It is one thing to stay focused on business logic, and another to review code and make it robust.
• PHP configuration : any code depends on PHP configuration or status, there is a good chance this code might need review. function_exists(), ini_get() or memory_limit() functions call are good spots to investigate. For example, if the code needs to check for the existence of a PHP function, then it is an old PHP function or an upcoming one. At the next version update of the code, this code may be abandoned, for the good of everything.

This list is short, so it is practical to remember and apply anytime I’m preparing some code to review. Except for the ‘Left for later’ code, all of them means that code will be removed, and the final result will be shorter. Over 1000 lines of code, I usually shave off 10 full lines of code. This is not much, but the feeling about the code quality is incredible.