Пример рефакторинга 
Недавно пообещал Тормозу отрефакторить несколько его функций. В этом посте выполняю обещание.
Не знаю на сколько стар тот код о котором пойдет речь ниже, поэтому предлагаю все нижеследующее рассмотреть просто как пищу для размышления (без перехода на личности). Итак, кандидат на рефакторинг номер один:
{
$hours = floor(($sec / 3600));
$days = floor(($sec / 86400));
$min = floor(($sec / 60) - $hours * 60);
if (!$min) $min = ”;
if ($days)
{
$hours = ($hours - ($days * 24));
}
if ($min) $min = $min.‘ ‘.plural($min, ‘минута’, ‘минуты’, ‘минут’);
if (!$hours) $hours = ”;
if ($hours) $hours = $hours.‘ ‘.plural($hours, ‘час’, ‘часа’, ‘часов’).‘ ‘;
if ($days)
{
$days = $days.‘ ‘.plural($days, ‘день’, ‘дня’, ‘дней’).‘ ‘;
$min = ”;
}
else $days = ”;
return $days.$hours.$min;
}
В этом коде меня смутило большое количество условий. Я искренне считаю, что количество условий должно стремиться к нулю (самая лучшая программа в которой вообще нет условий). И если какая-то функция содержит слишком много условных операторов - значит в коде что-то конкретно не так. Мой вариант такой:
return $n ? $n . ‘ ‘ . plural($n, $form1, $form2, $form5) : ”;
}
function age($sec)
{
$days = floor($sec / 86400);
$hours = floor($sec % 86400 / 3600);
$min = $days ? 0 : floor(($sec / 60) - $hours * 60);
$days = time2str($days, ‘день’, ‘дня’, ‘дней’);
$hours = time2str($hours, ‘час’, ‘часа’, ‘часов’);
$min = time2str($min, ‘минута’, ‘минуты’, ‘минут’);
При рефакторинге обратил внимание, что первоначальный вариант функции в некоторых случаях добавлял лишний пробел в конце возвращаемой строки, посчитав это багом решил избавиться от подобного поведения.
Идем далее, вот еще одна функция, которая точно так же содержит слишком много условий:
{
//define(’Path’, getcwd()); // должна определяться везде, кроме файла daos.php
$limit = 10000; // Количество попыток подключить файл
$sleep = 50; // Задержка перед попытками (в микросекундах)
$fileLock = Path."/var/$name.lock";
if (!file_exists(FileLock))
{
return True;
}
else // ждём и пробуем
{
$allowed = False;
$count = 0;
while(!$allowed OR $limit >= $count)
{
usleep($sleep); ++$count;
clearstatcache(); // а потому что file_exists() кэшируется!
if (!file_exists(FileLock)) $allowed = True; // выход из цикла
}
return $allowed;
}
} // allowed_RW()
Мой вариант такой (этот вариант не покрывал тестами, так что если заметите ошибку пишите в комментариях):
$fileLock = Path."/var/$name.lock";
$isBlocked = file_exists($fileLock);
while($isBlocked and $numTriesLeft–) { // Здесь $numTriesLeft с двумя минусами - типа декремент
usleep($waitTime);
clearstatecach(); // а потому что file_exists() кэшируется!
$isBlocked = file_exists($fileLock);
}
return $isBlocked ? false : true;
Несколько слов о том, что особенно не понравилось в первоначальном варианте:
Во-первых, формирование в условиях отрицательных утверждений: “Если файл НЕ существует” или “Если запись НЕ разрешена”. На мой взгляд гораздо лучше делать утвердительные условия: “Если файл заблокирован”.
Во-вторых, мало того, что в условиях используется отрицание, так оно еще и двойное:
{
//…
if (!file_exists(FileLock)) $allowed = True; // второе отрицание - !file_exists.
}
В-третьих, время от времени вместо инкремента лучше использовать декримент, как в случае с числом попыток, на мой взгляд обратный отсчет в данном случае выглядит намного понятнее.
В заключение, рассмотрим еще одну функцию (на закуску):
vw = Variables Write. Вот.{
$name = $data[‘type’]; // имя и тип массива данных
$fileName = Path."/var/$name.php";
$tmpName = Path.‘/var/’.rand(256,16384); // я знаю про tempnam()
$fileLock = Path."/var/$name.lock";
if (allowed_RW($name))
{
touch($fileLock);
$data = var_export($data, true);
$data = "<?php \$EOF=0; \$$name = $data; \$EOF=1; Return True;";
file_put_contents($tmpName, $data);
rename($tmpName, $fileName);
unlink($fileLock);
}
} // vw()
Здесь я просто предлагаю избавиться от явно лишнего условия, примерно вот так:
vw = Variables Write. Вот.{
$name = $data[‘type’]; // имя и тип массива данных
return allowed_RW($name) AND write_to_file($name, $data);
} // vw()
function write_to_file($name, $data) {
$fileName = Path."/var/$name.php";
$tmpName = Path.‘/var/’.rand(256,16384); // я знаю про tempnam()
$fileLock = Path."/var/$name.lock";
touch($fileLock);
$data = var_export($data, true);
$data = "<?php \$EOF=0; \$$name = $data; \$EOF=1; Return True;";
file_put_contents($tmpName, $data);
rename($tmpName, $fileName);
unlink($fileLock);
}
Ну вот… Теперь можете пинать меня ногами за предложенные изменения.
подписаться на блог
Лучшие комментарии
nekufa
Гость
Занятно Хорошо бы ещё заменить
return $isBlocked ? false : true;
return !$isBlocked;
а var_export очень удобно использовать так
file_put_contents(’test.php’, ‘<?php return ‘.var_export($data, true).’;’);
Соответственно где надо загрузить просто include ‘test.php’;
Не понятно зачем там $EOF = 0, $EOF = 1..
Всё равно если будет ошибка при парсинге свалится интерпретатор пхп и return true не поможет.
И ещё я бы поменял название функций allow_RW.
allow_RW - это какие действие. Например “разреши доступ”.
Метод для проверки чего-то хорошо называть isAllowed, isWritable (или is_allowed, is_writable - от стиля кодирования суть не меняется).
Метод для форматирования возраста getAge, либо getAgeString.
Код становится прозрачней и понятней
Андрей
Гость
Хоршо написано, но только пример не показательный, я считаю. Узкие места программы чаще сего не ветвения, а работа с Бди другие ресурсоемкие операции
Тормоз
Гость
Эх, жаль, что ты переписывал всё же древнюю функцию, а не из нового Daos. В целом почти со всеми изменениями согласен, спасибо.
Plural у меня такая избыточная получилась, потому что изначально была простенькой функцией только для часов и минут, а потом уже в последних версиях добавил дни. Причём, похоже, делал это в сонном состоянии
AllowRW твоя больше понравилась совершенно точно, лучше сделано, буду учится. Только сокращённые записи вида blabla ? True : False не нравятся, я на них всегда спотыкаюсь, неочевидные какие-то штуки.
Рефакторинг rw() сомнительный, не много смысла в избавлении от условия за счёт декомпозиции. Не понимаю, зачем в данном случае одну функцию разделять на две.
Ну и замечу, что в последней версии Daos функции чтения/записи совсем другие, там вообще другой механизм. Так что тут рефакторинг просто для удовольствия получается, использовать его всё равно негде.
P.S. Андрей, в данном случае рефакторинг производился для лучшего чтения кода, а не для удаления “узких мест”.
P.P.S. Евгений, у тебя парсер кавычки какие-то неправльные ставит в коде.
Evgeny Sergeev
Веб-разработчик, автор блога codeart.ru
Тормоз, я хотел именно с условиями взять что-нибудь, а новые функции по чтению записи их почти не используют.
> Рефакторинг rw() сомнительный, не много смысла в избавлении от условия за счёт декомпозиции. Не понимаю, зачем в данном случае одну функцию разделять на две.
На функции я бью с целью уменьшить количество задач решаемых в рамках одной функции. Иначе со временем функции начинают пухнуть (как у тебя с Plural получилось).
Я обычно делю названия для уровня бизнес логики и для технической реализации. В случае с чтением и записи я бы сделал следующие функции.
Бизне-логика:
load() - DAOS не должен знать технические особенности хранения данных. Они могут быть как в базе так и в файлах. Главное,чтобы load() возвращала массив данных. Откуда она его возьмет - ее проблемы.
Реализация(эти функции используются внутри load):
safe_write_array_to_file(…) - безопасный тип записи с использованием блокировки
write_array_to_file(…) - небезопасный без использования блокировки. Можно использовать повторно в тех случаях когда блокировка не нужна. (Это к вопросу разделения функций на две, который ты выше задавал)
>и замечу, что в последней версии Daos функции чтения/записи совсем другие, там вообще другой механизм. Так что тут рефакторинг просто для удовольствия получается, использовать его всё равно негде.
похожих мест у тебя много, ты часто используешь отрицательные конструкции вместо утвердительных, а они хуже читаются (восклицательный знак плохо заметен).
Вот пример:
if (!empty($linesOut))
можно заменить на
if (count($linesOut) > 0) или просто if ($linesOut).
Тормоз
Гость
Согласен, у меня такое бывало раньше постоянно и сейчас иногда случается. Утвердительные в большинстве случаев лучше, исправлюсь.
Тормоз
Гость
Но всё же в if (!empty(bla)) ничего плохого не вижу, оно вполне легко читается и понимается
Лучше уж, чем добавление сравнения с нулём. Однако, в некоторых случаях, естественно, лучше заменить на if (blabla). Не помню, почему там именно так сделал. Возможно, на то были причины.
nekufa
Гость
Занятно Хорошо бы ещё заменить
return $isBlocked ? false : true;
return !$isBlocked;
а var_export очень удобно использовать так
file_put_contents(’test.php’, ‘<?php return ‘.var_export($data, true).’;’);
Соответственно где надо загрузить просто include ‘test.php’;
Не понятно зачем там $EOF = 0, $EOF = 1..
Всё равно если будет ошибка при парсинге свалится интерпретатор пхп и return true не поможет.
И ещё я бы поменял название функций allow_RW.
allow_RW - это какие действие. Например “разреши доступ”.
Метод для проверки чего-то хорошо называть isAllowed, isWritable (или is_allowed, is_writable - от стиля кодирования суть не меняется).
Метод для форматирования возраста getAge, либо getAgeString.
Код становится прозрачней и понятней
Evgeny Sergeev
Веб-разработчик, автор блога codeart.ru
Насчет isBlocked не согласен, мне кажется, что отрицание - не самый лучший инструмент Php.
Насчет именования функций, согласен, что лучше изменить названия. В примерах не стал менять чтобы было понятно откуда что берется.
Тормоз
Гость
Nekufa, насчёт названий типа isAllowed согласен, спасибо за интересные замечания. А вот про include категорически нет, вообще. Твой вариант совсем не будет работать под нагрузками.
Читай тут - http://brokenbrake.biz/2010/10/16/refactoring-Daos-RW-func-2 (ну и другие статьи из этого цикла).
nekufa
Гость
“отрицание - не самый лучший инструмент php”
А в других языках инструмент “отрицание” лучше? Почему именно в PHP использовать отрицание плохо? А если везде плохо, то чем, именно?
Вообщем, мне кажется что чем меньше кода и чем он проще, тем лучше. Тернарный оператор по своей природе сложнее чем отрицание, так как функциональность его выше. Ну и даже меньшее количество символом намекают на то, что проанализирует человек это выражение быстрее.
Что касается моего варианта под нагрузками. Ясно, я не рассматривал варианты, когда файл часто меняется. Блокировка была не нужна, так как изменялись сгенерированные файлы (по сути кэш файловый). Про нагрузки - это скорее к вопросу о том, как правильно в него данные класть.
Поизучал все варианты, а чем не подошёл стандартный механизм блокировки файла? http://php.net/flock
У меня когда были “чудесные” идеи о хранении данных в ФС, я остановился на таком варианте и не припомню проблем. Или всё-таки что-то там не чисто ?
p.s. } // endOfFunction -> хорошая практика, но я бы запарился так писать каждый раз. или это среда какая-то добавляет автоматом?
Хотя, с другой стороны, опять же лишняя информация. одна функция, как правило, помещается на экране. Вообщем, спорно - это уже что-то из области стиля кодирования.
Evgeny Sergeev
Веб-разработчик, автор блога codeart.ru
nekufa,
Не совсем правильно выразился. Инструмент понятное дело не лучше и не хуже. Мне не нравится следующее:
1. запись в виде восклицательного знака не очень читабельна.
2. Отрицательные предложение воспринимать труднее утвердительных.
Есть еще один момент, который не сразу бросается в глаза - записи “return !$isBlocked;” и “return $isBlocked ? true : false” - имеют разные смысловые значения. Первая запись это отношение “является” (AllowRW является отрицанием isBlocked), вторая запись - это отношение “зависит” (значение allwoRW зависит от значения isBlocked).
А мне кажется, что код должен быть ровно такой длины, чтобы быть выразительным. Слишком короткий код малоинформативен, а слишком длинный трудночитаем.
nekufa
Гость
Все-таки я не корректно выразился. На первом месте, конечно, не то чтобы кода было меньше, а чтобы он был проще. В ваших рассуждениях есть рациональное зерно и вы правы.
А простота кода - понятие относительное. Каждому своё. Мне кажется, что return !$locked выглядит короче и понятней. Возможно потому, что мы везде пишем так.
Ну то есть
$someResult = in_array($value, $array) ? true : false;
для меня выглядит тяжелее чем
$someResult = !in_array($value, $array);
Особенно это заметно в небольших функциях, типа таких:
public function unitExists(Unit $unit) {
return in_array($unit->getId(), $this->getUnits()->keys);
}
Размазывать такие простые операции на несколько строчек смысла нет, а так выглядит компактней.
Тормоз
Гость
Евгений, судя по всему, противопоставление отрицания восклицательным знаком и тернарных операторов - это просто вкусовщина ) Тебе вот отрицания не нравятся, а Nekufa и я не перевариваем синтаксис тернарных.
Хотя, может и поспорить можно, всё же предполагаю, что в мозгу большинства людей отрицание обрабатывается на доли секунд быстрее, нежели этот тернарный синтаксис.
Nekufa, endOfFunction добавляется элементарно - пишется пара символов в начале, после автодополнение. Vim.
А flock действительно с проблемами, иначе и не было бы всей этой эпопеи с рефакторингом. Если интересно, можешь изучить мой опыт, а потом самостоятельно понаступать на эти грабли.
Тормоз
Гость
И, да, endOfFunction теперь для меня действительно лишнее. Просто я раньше злоупотреблял большими функциями, которые на экран совсем не влазили )
nekufa
Гость
Тормоз, ага - понял с локами. А если не секрет - что за задача была, где одновременная работа с ФС в несколько потоков необходима?
А ещё в качестве tmpName хорошо идут md5($name);
rand точно даст высокую вероятность повторение идентификатора.
Синтаксис тернарного оператора я вполне себе перевариваю:)
Просто получается какой-то “капитан очевидность”.
Результат есть, его надо просто правильно использовать, а не вычислять новое. Алгоритмы для меня выглядят вот так:
1. Изменять файл можно если нет блокировки.
return !$fileIsLocked;
2. Если файл заблокирован, то изменять можно, иначе нельзя.
return $fileIsLocked ? true : false;
То есть для меня это как лишнее действие.
На самом деле, как правильнее и почему мы никогда не выясним.
Это действительно дело вкуса
Тормоз
Гость
Как ты читал, если не понял, что за задача была?
Задача проста и сложна одновременно: нужно чтобы Daos надёжно работал в любых условиях. Теперь эта задача решена.
А смысла в md5 для временного имени вообще не вижу. В каком смысле “хорошо идут”, чего хорошего-то?
nekufa
Гость
Ну я глянул исходники, понял что там речь идёт о блокировке.
Из той статьи видно, что эта часть системы оптимизируется. Почему бы банально в мускуле не хранить?
Для чего в даосе это используется, в какой подсистеме - из статьи не очевидно. Также не понятно, почему было решено выбрать фс как хранилище
Функция rand как источник для временного имени - это даже хуже. Хорошо идут - много где используется. Самый просто способ получить уникальный идентификатор контента - это взять от него хэш.
Тормоз
Гость
Чем rand хуже? Давай с аргументами.
nekufa
Гость
rand хуже тем, что это случайно число.
Хэш от строки - уникальный идентификатор относящийся именно к этой строке.
Другими словами, вероятность того, что идентификатор используется каким-то параллельным процессом есть.
В случае с использованием хэш-функции идентификатор совпадёт только если данные идентичны.
Использовать ранд - как уникальный идентификатор не стоит.
Я про листинг http://pastie.org/1223867
$tmpName = Path.’/var/’.rand(256,16384); // я знаю про tempnam()
Здесь ранд может указать на существующий файл.
Запись в параллельном процессе в него может идти из другого места с , возможно, данными для другого имени файла.
Тормоз
Гость
Вероятность такого события слишком мала.
nekufa
Гость
Согласен, просто она есть.
Чем использовать хэш лучше - тем, что вероятности изначально такой просто нет. Конечно, это всё мелочи. И в текущем проекте нет никаких ужасов и вообще причин отказывать от текущей реализации.
Но для себя я отметил что это хороший механизм, хотя, конечно, и не панацея
Ещё я бы посоветовал отказаться от функций как они есть, а логику раскладывать по классам. Например, FileSystem с методами write, read, isWritable, isReadable..
Или даже сокращённо fs, раз уж вы так любите сокращения
(судя по vw и allowed_RW). Сделать там статические методы.
Вызов fs::read($name) - пишется короче, выглядит не хуже, вроде, чем include_flock($name);
Из плюсов - По любой строчке всегда видно - вызывается стандартная функция или какая-то “своя”. При дебаге - беглый взгляд по исходнику будет отмечать эти места. Как то, где ещё возможна причина.
Ещё в том же исходнике видно что вы используете глобальные переменные - это тоже плохо на самом деле.
Но вообще, это все такие сомнительные советы.. Нигде подобного не читал и не думаю что это интересно будет. Субъективно что-то хорошо для кого-то, что-то плохо.
Мне например не нравится когда первую скобку в условии ставят на следующую строку
Визуально код длиннее становиться. А ещё я всегда тру закрывающийся тэг (?>), если увижy где 
Тормоз
Гость
Ко мне лучше на “ты” всегда. Но я могу на “вы”, если у Вас так принято
В ООП я только начинаю врубаться, вот как раз в процессе написания своего первого класса. Почти автоматически и глобальные переменные уйдут.
Насчёт первой скобке на отдельной строке - оно удобней на самом деле, я много думал о различии стилей. Намного удобнее в редактировании, и при подсветке скобок уровень вложенности кода всегда проще увидеть, т. к. с вариантом, когда скобка на той же строке, что предшествующее выражение, никакого уровня они своим положением не показывают. Так что это мой однозначный осознанный продуманный выбор.
Тормоз
Гость
Скобке = скобки
nekufa
Гость
Да не, на ты легче - у нас так не принято, просто к незнакомым людям обращаться легче на вы
Мне кажется, что уровень вложенности показан отступом содержимого блока. А так на экран больше влезает текста. И методы короче) Вообщем, дело привычки. Да и переучиваться тяжеловато.
Да ладно, скобке - это ок
Leave a Reply