// codeart.ru / Вопрос/Ответ / Пример рефакторинга Форум

Пример рефакторинга rss подписка

Автор: Evgeniy Sergeev

Недавно пообещал Тормозу отрефакторить несколько его функций. В этом посте выполняю обещание.

Не знаю на сколько стар тот код о котором пойдет речь ниже, поэтому предлагаю все нижеследующее рассмотреть просто как пищу для размышления (без перехода на личности). Итак, кандидат на рефакторинг номер один:


function age($sec)
{
$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;
}

В этом коде меня смутило большое количество условий. Я искренне считаю, что количество условий должно стремиться к нулю (самая лучшая программа в которой вообще нет условий). И если какая-то функция содержит слишком много условных операторов — значит в коде что-то конкретно не так. Мой вариант такой:


function time2str($n, $form1, $form2, $form5) {
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, 'минута', 'минуты', 'минут');

return trim(implode(' ', array($days,$hours,$min)));
}

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

Идем далее, вот еще одна функция, которая точно так же содержит слишком много условий:

function allowed_RW($name)
{
//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()

Мой вариант такой (этот вариант не покрывал тестами, так что если заметите ошибку пишите в комментариях):

function allow_RW($name, $numTriesLeft = 10000, $waitTime = 50) {
$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;

Несколько слов о том, что особенно не понравилось в первоначальном варианте:

Во-первых, формирование в условиях отрицательных утверждений: «Если файл НЕ существует» или «Если запись НЕ разрешена». На мой взгляд гораздо лучше делать утвердительные условия: «Если файл заблокирован».

Во-вторых, мало того, что в условиях используется отрицание, так оно еще и двойное:

while(!$allowed OR $limit >= $count) // первое отрицание - !$allowed
{
//...
if (!file_exists(FileLock)) $allowed = True; // второе отрицание - !file_exists.
}

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

В заключение, рассмотрим еще одну функцию (на закуску):


function vw($data) // Это не VolksWagen :) 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 = "
Здесь я просто предлагаю избавиться от явно лишнего условия, примерно вот так:

function vw($data) // Это не VolksWagen :) 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 = "

Ну вот... Теперь можете пинать меня ногами за предложенные изменения.

  1. Продолжая тему рефакторинга DAOS
  2. Про скрытую сортировку и фильтрацию массивов
  3. Создание и раскрутка сайта,заработок в сети… » Blog Archive » Продолжая тему рефакторинга DAOS

    Лучшие комментарии

  1. Занятно Хорошо бы ещё заменить

    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.

    Код становится прозрачней и понятней

  1. Хоршо написано, но только пример не показательный, я считаю. Узкие места программы чаще сего не ветвения, а работа с Бди другие ресурсоемкие операции

  2. Эх, жаль, что ты переписывал всё же древнюю функцию, а не из нового Daos. В целом почти со всеми изменениями согласен, спасибо.

    Plural у меня такая избыточная получилась, потому что изначально была простенькой функцией только для часов и минут, а потом уже в последних версиях добавил дни. Причём, похоже, делал это в сонном состоянии :)

    AllowRW твоя больше понравилась совершенно точно, лучше сделано, буду учится. Только сокращённые записи вида blabla ? True : False не нравятся, я на них всегда спотыкаюсь, неочевидные какие-то штуки.

    Рефакторинг rw() сомнительный, не много смысла в избавлении от условия за счёт декомпозиции. Не понимаю, зачем в данном случае одну функцию разделять на две.

    Ну и замечу, что в последней версии Daos функции чтения/записи совсем другие, там вообще другой механизм. Так что тут рефакторинг просто для удовольствия получается, использовать его всё равно негде.

    P.S. Андрей, в данном случае рефакторинг производился для лучшего чтения кода, а не для удаления «узких мест».

    P.P.S. Евгений, у тебя парсер кавычки какие-то неправльные ставит в коде.

  3. Тормоз, я хотел именно с условиями взять что-нибудь, а новые функции по чтению записи их почти не используют.

    > Рефакторинг 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).

  4. Согласен, у меня такое бывало раньше постоянно и сейчас иногда случается. Утвердительные в большинстве случаев лучше, исправлюсь.

  5. Но всё же в if (!empty(bla)) ничего плохого не вижу, оно вполне легко читается и понимается :) Лучше уж, чем добавление сравнения с нулём. Однако, в некоторых случаях, естественно, лучше заменить на if (blabla). Не помню, почему там именно так сделал. Возможно, на то были причины.

  6. Занятно Хорошо бы ещё заменить

    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.

    Код становится прозрачней и понятней

  7. Насчет isBlocked не согласен, мне кажется, что отрицание — не самый лучший инструмент Php.
    Насчет именования функций, согласен, что лучше изменить названия. В примерах не стал менять чтобы было понятно откуда что берется.

  8. Nekufa, насчёт названий типа isAllowed согласен, спасибо за интересные замечания. А вот про include категорически нет, вообще. Твой вариант совсем не будет работать под нагрузками.

    Читай тут — http://brokenbrake.biz/2010/10/16/refactoring-Daos-RW-func-2 (ну и другие статьи из этого цикла).

  9. «отрицание — не самый лучший инструмент php»
    А в других языках инструмент «отрицание» лучше? Почему именно в PHP использовать отрицание плохо? А если везде плохо, то чем, именно?

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

    Что касается моего варианта под нагрузками. Ясно, я не рассматривал варианты, когда файл часто меняется. Блокировка была не нужна, так как изменялись сгенерированные файлы (по сути кэш файловый). Про нагрузки — это скорее к вопросу о том, как правильно в него данные класть.

    Поизучал все варианты, а чем не подошёл стандартный механизм блокировки файла? http://php.net/flock
    У меня когда были «чудесные» идеи о хранении данных в ФС, я остановился на таком варианте и не припомню проблем. Или всё-таки что-то там не чисто ?

    p.s. } // endOfFunction -> хорошая практика, но я бы запарился так писать каждый раз. или это среда какая-то добавляет автоматом?
    Хотя, с другой стороны, опять же лишняя информация. одна функция, как правило, помещается на экране. Вообщем, спорно — это уже что-то из области стиля кодирования.

  10. nekufa,

    • А в других языках инструмент “отрицание” лучше? Почему именно в PHP использовать отрицание плохо? А если везде плохо, то чем, именно?

    Не совсем правильно выразился. Инструмент понятное дело не лучше и не хуже. Мне не нравится следующее:
    1. запись в виде восклицательного знака не очень читабельна.
    2. Отрицательные предложение воспринимать труднее утвердительных.

    Есть еще один момент, который не сразу бросается в глаза — записи «return !$isBlocked;» и «return $isBlocked ? true : false» — имеют разные смысловые значения. Первая запись это отношение «является» (AllowRW является отрицанием isBlocked), вторая запись — это отношение «зависит» (значение allwoRW зависит от значения isBlocked).

    • Вообщем, мне кажется что чем меньше кода и чем он проще, тем лучше.

    А мне кажется, что код должен быть ровно такой длины, чтобы быть выразительным. Слишком короткий код малоинформативен, а слишком длинный трудночитаем.

  11. Все-таки я не корректно выразился. На первом месте, конечно, не то чтобы кода было меньше, а чтобы он был проще. В ваших рассуждениях есть рациональное зерно и вы правы.

    А простота кода — понятие относительное. Каждому своё. Мне кажется, что 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);
    }

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

  12. Евгений, судя по всему, противопоставление отрицания восклицательным знаком и тернарных операторов — это просто вкусовщина ) Тебе вот отрицания не нравятся, а Nekufa и я не перевариваем синтаксис тернарных.

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

    Nekufa, endOfFunction добавляется элементарно — пишется пара символов в начале, после автодополнение. Vim.

    А flock действительно с проблемами, иначе и не было бы всей этой эпопеи с рефакторингом. Если интересно, можешь изучить мой опыт, а потом самостоятельно понаступать на эти грабли.

  13. И, да, endOfFunction теперь для меня действительно лишнее. Просто я раньше злоупотреблял большими функциями, которые на экран совсем не влазили )

  14. Тормоз, ага — понял с локами. А если не секрет — что за задача была, где одновременная работа с ФС в несколько потоков необходима?
    А ещё в качестве tmpName хорошо идут md5($name);
    rand точно даст высокую вероятность повторение идентификатора.

    Синтаксис тернарного оператора я вполне себе перевариваю:)
    Просто получается какой-то «капитан очевидность».
    Результат есть, его надо просто правильно использовать, а не вычислять новое. Алгоритмы для меня выглядят вот так:

    1. Изменять файл можно если нет блокировки.
    return !$fileIsLocked;

    2. Если файл заблокирован, то изменять можно, иначе нельзя.
    return $fileIsLocked ? true : false;

    То есть для меня это как лишнее действие.
    На самом деле, как правильнее и почему мы никогда не выясним.
    Это действительно дело вкуса :)

  15. Как ты читал, если не понял, что за задача была? :)
    Задача проста и сложна одновременно: нужно чтобы Daos надёжно работал в любых условиях. Теперь эта задача решена.

    А смысла в md5 для временного имени вообще не вижу. В каком смысле «хорошо идут», чего хорошего-то?

  16. Ну я глянул исходники, понял что там речь идёт о блокировке.
    Для чего в даосе это используется, в какой подсистеме — из статьи не очевидно. Также не понятно, почему было решено выбрать фс как хранилище :) Из той статьи видно, что эта часть системы оптимизируется. Почему бы банально в мускуле не хранить?

    Функция rand как источник для временного имени — это даже хуже. Хорошо идут — много где используется. Самый просто способ получить уникальный идентификатор контента — это взять от него хэш.

  17. Чем rand хуже? Давай с аргументами.

  18. rand хуже тем, что это случайно число.
    Хэш от строки — уникальный идентификатор относящийся именно к этой строке.
    Другими словами, вероятность того, что идентификатор используется каким-то параллельным процессом есть.
    В случае с использованием хэш-функции идентификатор совпадёт только если данные идентичны.

    Использовать ранд — как уникальный идентификатор не стоит.
    Я про листинг http://pastie.org/1223867
    $tmpName = Path.’/var/’.rand(256,16384); // я знаю про tempnam()

    Здесь ранд может указать на существующий файл.
    Запись в параллельном процессе в него может идти из другого места с , возможно, данными для другого имени файла.

  19. Вероятность такого события слишком мала.

  20. Согласен, просто она есть.

    Чем использовать хэш лучше — тем, что вероятности изначально такой просто нет. Конечно, это всё мелочи. И в текущем проекте нет никаких ужасов и вообще причин отказывать от текущей реализации.

    Но для себя я отметил что это хороший механизм, хотя, конечно, и не панацея :)

    Ещё я бы посоветовал отказаться от функций как они есть, а логику раскладывать по классам. Например, FileSystem с методами write, read, isWritable, isReadable..

    Или даже сокращённо fs, раз уж вы так любите сокращения :) (судя по vw и allowed_RW). Сделать там статические методы.
    Вызов fs::read($name) — пишется короче, выглядит не хуже, вроде, чем include_flock($name);

    Из плюсов — По любой строчке всегда видно — вызывается стандартная функция или какая-то «своя». При дебаге — беглый взгляд по исходнику будет отмечать эти места. Как то, где ещё возможна причина.

    Ещё в том же исходнике видно что вы используете глобальные переменные — это тоже плохо на самом деле.

    Но вообще, это все такие сомнительные советы.. Нигде подобного не читал и не думаю что это интересно будет. Субъективно что-то хорошо для кого-то, что-то плохо.

    Мне например не нравится когда первую скобку в условии ставят на следующую строку :) Визуально код длиннее становиться. А ещё я всегда тру закрывающийся тэг (?>), если увижy где :)

  21. Ко мне лучше на «ты» всегда. Но я могу на «вы», если у Вас так принято :)

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

    Насчёт первой скобке на отдельной строке — оно удобней на самом деле, я много думал о различии стилей. Намного удобнее в редактировании, и при подсветке скобок уровень вложенности кода всегда проще увидеть, т. к. с вариантом, когда скобка на той же строке, что предшествующее выражение, никакого уровня они своим положением не показывают. Так что это мой однозначный осознанный продуманный выбор.

  22. Скобке = скобки :(

  23. Да не, на ты легче — у нас так не принято, просто к незнакомым людям обращаться легче на вы :)

    Мне кажется, что уровень вложенности показан отступом содержимого блока. А так на экран больше влезает текста. И методы короче) Вообщем, дело привычки. Да и переучиваться тяжеловато.

    Да ладно, скобке — это ок :)

Leave a Reply