Code Review

Ревью кода проекта на 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;
}

Ревью кода проекта на Windows Phone 7. #1

Виталий Басов

Как и в большинстве компаний, мы проводим внутреннюю проверку качества кода. Без сомнения эта практика полезна как начинающим разработчикам так и уже устоявшимся профессионалам.

Ниже первый материал из новой серии статей.

Шаблон MVVM

Пример 1

NewsItemViewModel newsItemViewModel =
ViewModelFactory.Instance.GetViewModel<NewsItem, NewsItemViewModel>(newsItem);
newsItemViewModel.Width = 310;
mainNews.Children.Add(newsItemViewModel);

Пример 2

var link = new Hyperlink {
MouseOverForeground = new SolidColorBrush(Color.FromArgb(155, 0, 102, 204)),
Foreground = new SolidColorBrush(Color.FromArgb(255, 0, 102, 204))
};

Что не так с этими примерами? Казалось бы обычная инициализация контролов. Но если задуматься о том, что этот код находится во ViewModel, то становится видна проблема. Во-первых, ViewModel занимается не своим делом – работает с визуальными аспектами. Во-вторых, в коде жестко зашиты значения и для потребителя этого контрола теряется возможность влиять на внешний вид.
Согласно шабону MVVM, все визуальные аспекты должны определяться во View. Это продиктовано соображениями декомпозиции и слабой связанности. Для установки свойств предпочтительным способом является декларативный.

Константы

var Items = new List<Item>
{
new RubricItem
{
Title = “Новости”,
SectionType = SectionType.News
},

В данном примере свойство Title инициализируется литералом по месту. В общем случае такой прием является плохой практикой. Когда такие строки предназначены для пользовательского интерфейса, последующая локализация может превратиться в головную боль. Чтобы облегчить свою долю, лучше оформлять строки в виде констант. Если говорить в общем, то использование констант улучшает читаемость и облегчает рефакторинг.

Перечисления

Для целей идентификации, вместо констант более оправданно использовать перечисления. Тем самым мы понижаем вероятность внесения ошибок. Как и в случае с константами, перечисление определяется однажды, а в качестве бонуса мы получаем уникальность ключа. Если позволяет логика приложения, то в качестве значения ключа лучше использовать целые числа, а не строки – сравнение строк более тяжелая операция.

В примере ниже, класс CacheKeys следует определить в виде перечисления и использовать целочисленный тип в качестве базового.

public static class CacheKeys
{
/*Items*/
public const string NewsItemsKey = “NewsItems”;
public const string RubricItemsKey = “RubricItems”;
public const string CommentItemsKey = “CommentItems”;
public const string OfftopicItemsKey = “OfftopicItems”;
public const string GalleryItemsKey = “GalleryItems”;
public const string Favorites = “Favorites”;

/*TopicItems*/
public const string GalleryKey = “GalleryItems”;
public const string OfftopicKey = “OfftopicItems”;
public const string CommentKey = “CommentItems”;
public const string NewsKey = “News”;

/*Main*/
public const string MainTopicsKey = “MainTopics”;
public const string RubricsKey = “Rubrics”;

/*Rubric*/
public const string RubricTopicsKey = “RubricTopics”;
}

Copy & Paste

При разработке часто возникают ситуации, когда один и тот же алгоритм с небольшими вариациями определяется в нескольких местах. Такие случаи надо выявлять и приводить к одному знаменателю. В зависимости от ситуации, решать эту задачу можно по-разному:

  1. Оформить повторяющийся участок кода в виде функции, возможно с параметрами
  2. Создать виртуальную функцию с переопределением в наследнике, когда в определенной последовательности работают базовый вариант и/или вариант наследника.
  3. Шаблонный метод с использованием наследования, контракта или делегатов

Проблему ниже можно решить через обобщенную функцию вида IList DeserializeItems()

public IList<CommentItem> DeserializeCommentItems()
{
List<CommentItem> items = null;
var targetFileName = String.Format(“{0}/{1}.dat”, FolderName, CacheKeys.CommentItemsKey);

if (IsoFile.FileExists(targetFileName))
{
try
{
using (IsolatedStorageFileStream sourceStream = IsoFile.OpenFile(targetFileName, FileMode.Open))
{
items = (List<CommentItem>) CommentItemSerializer.ReadObject(sourceStream);
}
}
catch (Exception)
{
//TODO: log
}
}

return items;
}

public IList<NewsItem> DeserializeNewsItems()
{
List<NewsItem> items = null;
var targetFileName = String.Format(“{0}/{1}.dat”, FolderName, CacheKeys.NewsItemsKey);

if (IsoFile.FileExists(targetFileName))
{
try
{
using (IsolatedStorageFileStream sourceStream = IsoFile.OpenFile(targetFileName, FileMode.Open))
{
items = (List<NewsItem>)NewsItemSerializer.ReadObject(sourceStream);
}
}
catch (Exception)
{
//TODO: log
}
}

return items;
}

Функциональная декомпозиция

News news = News.Any(n => n.Id == id) ? News.FirstOrDefault(n => n.Id == id) : DeserializeNews(id);

private News DeserializeNews(string id)
{
return IsolatedStorageDataService.Instance.DeserializeNews(id);
}

В даном примере, функция DeserializeNews занимается банальным пробросом вызова и используется только в одном месте. Такая функция выглядит неоправданным усложнением.

Выполнять декомпозицию кода на отдельные функции имеет смысл в случае:

  1. Повторного использования
  2. Разбиения излишне большой функции для повышения читаемости
  3. Выделения логически завершенного действия

Не стоит заниматься функциональной декомпозицией на таком уровне заранее. Лучше чтобы этот процесс происходил естественным образом в ходе разработки. В таком случае функции будут появляться по мере необходимости.

Компонентная декомпозиция

public override void LoadData()
{
OnDataLoading();
NewsSectionManager.Instance.LoadNews(result =>
{
if (result == null)
{
ShowNoConnectionText();
CacheManager.Instance.SetState(CacheKeys.NewsItemsKey, true);
return;
}

Items.Clear();
foreach (NewsItem item in result)
{
Items.Add(ViewModelFactory.Instance.GetViewModel<NewsItem, NewsItemViewModel>(item));
OnDataLoaded();
if (CacheManager.Instance.NeedInvalidation(CacheKeys.NewsItemsKey))
{
ShowNoConnectionText();
}
});

if (NewsSectionManager.Instance.CacheLoaded)
{
InvalidateCommand.Execute();
}
}

В данном примере видно, что часть логики работы компонента для кэширования данных, находится во ViewModel, что конечно же не правильно. Хорошо спроектированный компонент не должен выставлять детали внутренней реализации, и тем более отдавать ее вовне.

Паттерн Фабричный метод

foreach (NewsItem item in result)
{
Items.Add(ViewModelFactory.Instance.GetViewModel<NewsItem, NewsItemViewModel>(item));
}

Задача фабричного метода – создать экземпляр подходящего класса на основе входных данных. Здесь же вызывающий сам решает какой класс надо сконструировать, что не соответствует применяемому шаблону. Плюс зачем то уточняется тип аргумента через параметр шаблона. Либо неверно выбрано название класса либо неверно применен фабричный метод.