Ревью кода проекта на PHP и JavaScript. #1

Владимир Долгополов Продолжаем публиковать результаты анализа исходных кодов наших текущих проектов. Под катом рассмотрены 10 ошибок в проекте на PHP.

1. Magic string в коде следует заменять на константы класса

Преимущества: среда разработки сама подскажет имя константы, значение меняется в одном месте. Неправильно:
public static function getPlayMoneyLimit() { $pointsLimit = self::poker_get_option('RequestPlayMoneyLimit'); return (int) $pointsLimit->message(); }
Правильно:
public static function getPlayMoneyLimit() { $pointsLimit = self::poker_get_option(self::REQUEST_PLAY_MONEY_LIMIT_OPTION); return (int) $pointsLimit->message(); }

2. Настройки следует выносить в отдельный конфиг файл, а не хранить в коде.

Например, логины, пароли, пути до файлов, параметры приложения. Все то, что надо менять при настройке приложения в другом месте. Неправильно (some_file.php):
define('PAYMENT_QUICKPAY_MERCHANT_ID', '5afab57b-33ef-4c59-03cd-9f7844f53118'); define('PAYMENT_QUICKPAY_WEBSITE_ID', '346baa5d-265a-479c-c588-4c6ee7c5762d');
Правильно (config.ini):
payment_quickpay_merchant_id = 5afab57b-33ef-4c59-03cd-9f7844f53118 payment_quickpay_website_id = 346baa5d-265a-479c-c588-4c6ee7c5762d

3. Функция должна возвращать данные только одного типа или null

Неправильно:
function dateField($dateField = null){ if (!isset($this->_dateField)) { $this->_dateField = ''; } if (is_null($dateField)) { return false; } else { $this->_dateField = $dateField; } return $this->_dateField; }
Правильно:
function dateField($dateField = null){ if (!isset($this->_dateField)) { $this->_dateField = ''; } if (is_null($dateField)) { return null; } else { $this->_dateField = $dateField; } return $this->_dateField; }

4. Описки исправляем сразу, иначе code completion разнесет их по всему проекту

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 в качестве ключа массива следует использовать, только если есть 100% уверенность, что не будет двух и более событий, произошедших в одну секунду.

6. Следует проверять существование ключа перед использованием в публичных методах

Иначе “Notice: Undefined index:”. В private и protected методах можно проверку опустить, если мы на 100% уверены, что этот ключ там есть. Неправильно:
public static function createTable($data) { $password = $data['password']; unset($data['password']); $tableInfo = new TableInfo(); … }
Правильно:
public static function createTable($data) { if (isset($data['password'])) { $password = $data['password'];  } else { $password = ''; } unset($data['password']); $tableInfo = new TableInfo(); … }

7. Javascript. Проверка переменной на существование (defined) должна быть через typeof

Неправильно:
setBlockId : function(blockId) { if (blockId != undefined) { this._blockId = blockId; } else{ this._blockId = this.DEFAULT_BLOCK_ID; } },
Правильно:
setBlockId : function(blockId) { if (typeof blockId != 'undefined') { this._blockId = blockId; } else{ this._blockId = this.DEFAULT_BLOCK_ID; } },
Потому что undefined не зарезервированное слово, всего лишь глобальная переменная, которая может быть переопределена. Поэтому не следует её использовать, только если в случае вы явно определили её значение в текущем контексте. Можно написать в начале скрипта “undefined = 1” и все сломается.

8. Исключения следует логировать, а не отображать пользователю

Неправильно:
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; } }
Правильно:
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. Экранирование переменных в html-шаблонах

Неправильно:
<html> <head> </head> <body> Player <?php echo $player->f_family_name . ' ' . $player->f_name . ' (' . $player->f_nick . ')'; ?> has uploaded new avatar. </body> </html>
Правильно:
<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. Оптимистичное допущение об успешности выполнения. Следует проверить результат и отреагировать на ошибку

Неправильно:
public function clearApprovedAvatar() { $this->_davRequest($this->getApprovedAvatarUrl(), 'DELETE'); return true; }
Правильно:
public function clearApprovedAvatar() { if ($this->_davRequest($this->getApprovedAvatarUrl(), 'DELETE')) { return true; }; Logger::log('Error on deletion ' . $this->getApprovedAvatarUrl()); return false; }