Владимир Долгополов
Продолжаем публиковать результаты анализа исходных кодов наших текущих проектов. Под катом рассмотрены 10 ошибок в проекте на PHP.
1. Magic string в коде следует заменять на константы класса
Преимущества: среда разработки сама подскажет имя константы, значение меняется в одном месте.
Неправильно:
public static function getPlayMoneyLimit() {
$pointsLimit = self::poker_get_option(‘RequestPlayMoneyLimit’);
return (int) $pointsLimit->message();
}
$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();
}
$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’);
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
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;
}
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;
}
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;
}
$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();
…
}
{
$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();
…
}
{
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;
}
},
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;
}
},
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;
}
}
{
$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);
}
}
{
$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>
<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>
<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;
}
$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;
}
if ($this->_davRequest($this->getApprovedAvatarUrl(), ‘DELETE’)) {
return true;
};
Logger::log(‘Error on deletion ‘ . $this->getApprovedAvatarUrl());
return false;
}