On Time, On Point, On Budget!

Code review for project on PHP and JavaScript. #1

Vladimir Dolgopolov We continue to publish the results of the analysis of our current projects source codes. Below 10 errors in project on PHP are reviewed.

1. Magic string in code should be replaced by class constants

Advantages: development environment will offer the constant name, the value is changed in one place. Wrong:
public static function getPlayMoneyLimit() { $pointsLimit = self::poker_get_option('RequestPlayMoneyLimit'); return (int) $pointsLimit->message(); }
Right:
public static function getPlayMoneyLimit() { $pointsLimit = self::poker_get_option(self::REQUEST_PLAY_MONEY_LIMIT_OPTION); return (int) $pointsLimit->message(); }

2. Settings should be placed in a separate config file and not stored in the code.

For example, logins, passwords, file paths, application parameters. All that needs to be changed when configuring the application in another location. Wrong (some_file.php):
define('PAYMENT_QUICKPAY_MERCHANT_ID', '5afab57b-33ef-4c59-03cd-9f7844f53118'); define('PAYMENT_QUICKPAY_WEBSITE_ID', '346baa5d-265a-479c-c588-4c6ee7c5762d');
Right (config.ini):
payment_quickpay_merchant_id = 5afab57b-33ef-4c59-03cd-9f7844f53118 payment_quickpay_website_id = 346baa5d-265a-479c-c588-4c6ee7c5762d

3. The function should return the data of only one type or null

Wrong:
function dateField($dateField = null){ if (!isset($this->_dateField)) { $this->_dateField = ''; } if (is_null($dateField)) { return false; } else { $this->_dateField = $dateField; } return $this->_dateField; }
Right:
function dateField($dateField = null){ if (!isset($this->_dateField)) { $this->_dateField = ''; } if (is_null($dateField)) { return null; } else { $this->_dateField = $dateField; } return $this->_dateField; }

4. Erratums should be corrected immediately, otherwise code completion will spread them throughout the project

function headDefatults(){ $re = array(); $heads = array_keys($this->headArray()); foreach ($heads as $key){ $re[$key] = 0; } return $re; } function dataDefaults($label){ $re = array( 'label' => $label, 'data' => $this->headDefatults(), 'total' => 0, ); return $re; }

5. Timestamp as the array key should be used only in case of 100% certainty that there won’t be two or more events occurred in one second

6. You should check the existence of the key before using it in public methods

Otherwise “Notice: Undefined index:”. In private and protected methods checking can be omitted if we are 100% sure that this key is there. Wrong:
public static function createTable($data) { $password = $data['password']; unset($data['password']); $tableInfo = new TableInfo(); … }
Right:
public static function createTable($data) { if (isset($data['password'])) { $password = $data['password'];  } else { $password = ''; } unset($data['password']); $tableInfo = new TableInfo(); … }

7. Javascript. Verifying the existence of a variable (defined) should be done using typeof

Wrong:
setBlockId : function(blockId) { if (blockId != undefined) { this._blockId = blockId; } else{ this._blockId = this.DEFAULT_BLOCK_ID; } },
Right:
setBlockId : function(blockId) { if (typeof blockId != 'undefined') { this._blockId = blockId; } else{ this._blockId = this.DEFAULT_BLOCK_ID; } },
Because undefined is not a reserved word, just a global variable that can be overridden. Therefore, you shouldn’t use it, or use it only when you explicitly has defined its value in the current context. “undefined = 1” can be written at the beginning of the script and everything will be broken.

8. Exceptions should be logged but not be displayed for the user

Wrong:
public function getActionsListAsJson() { $serializer = Zend_Serializer::factory('Json'); try { $actionsIds = array(); foreach($this->getActionList() as $action){ $actionsIds[] = $action->getSoldObject(); } return $serializer->serialize($actionsIds); } catch (Zend_Serializer_Exception $e) { echo $e; } }
Right:
public function getActionsListAsJson() { $serializer = Zend_Serializer::factory('Json'); try { $actionsIds = array(); foreach($this->getActionList() as $action){ $actionsIds[] = $action->getSoldObject(); } return $serializer->serialize($actionsIds); } catch (Zend_Serializer_Exception $e) { Logger::log($e); } }

9. Escape variables in the html-templates

Wrong:
<html> <head> </head> <body> Player <?php echo $player->f_family_name . ' ' . $player->f_name . ' (' . $player->f_nick . ')'; ?> has uploaded new avatar. </body> </html>
Right:
<html> <head> </head> <body> Player <?php echo $this->escape($player->f_family_name) . ' ' . $this->escape($player->f_name) . ' (' . $this->escape($player->f_nick) . ')'; ?> has uploaded new avatar. </body> </html>

10. Optimistic assumption about the success of execution. You should check the result and respond to the error

Wrong:
public function clearApprovedAvatar() { $this->_davRequest($this->getApprovedAvatarUrl(), 'DELETE'); return true; }
Right:
public function clearApprovedAvatar() { if ($this->_davRequest($this->getApprovedAvatarUrl(), 'DELETE')) { return true; }; Logger::log('Error on deletion ' . $this->getApprovedAvatarUrl()); return false; }
This entry was posted on Thursday, July 26th, 2012 at 8:23 am and is filed under Code Review, PHP.