• Главная»
  • Уроки»
  • PHP»
  • Рефакторинг унаследованного кода: Часть 2 - Магические строки и константы

Этот урок связан с проектом Рефакторинг унаследованного кода

Рефакторинг унаследованного кода: Часть 2 - Магические строки и константы

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

Впервые мы встретились с унаследованным кодом в нашей предыдущей статье. Затем запустили код, чтобы сформировать мнение о нём, и создали набор тестов - “Золотую сборку”, - для того, чтобы у нас был надёжный и безопасный набор проверок для будущих изменений. Мы продолжим работу с этим кодом, и найти вы его можете в директории trivia/php_start. Другая директория trivia/php_start содержит завершённый код данного урока.

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

Магические строки

Магические строки - это строки, которые напрямую используются в различных выражениях, без присваивания в переменные. Эти строки имеют особое значение для автора кода, вместо того, чтобы занести их в переменную с корректным названием, автор использовал их в явном виде, посчитав, что строка очевидна сама по себе.

Определяемся с первым кандидатом на замену

Начнём с файла Game.php, и попробуем определить строки. Если вы используете IDE (как, в общем-то, и должны), или небольшой текстовый редактор с возможностью подсветки синтаксиса, поиск строк не станет проблемой. Вот изображение того, как код выглядит на моем экране:

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

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

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

for ($i = 0; $i < 50; $i++) {
        array_push($this->popQuestions, "Pop Question " . $i);
        array_push($this->scienceQuestions, ("Science Question " . $i));
        array_push($this->sportsQuestions, ("Sports Question " . $i));
        array_push($this->rockQuestions, $this->createRockQuestion($i));
    }
}

function createRockQuestion($index) {
    return "Rock Question " . $index;
}

Давайте проанализируем строки 34-42, которые представлены на вышеприведённом фрагменте. Для вопросов в области поп, науки и спорта используется простая конкатенация. Но вот составление вопроса в области рока вынесено в отдельный метод. На ваш взгляд, достаточно ли понятны эти строки, чтобы их можно было оставить в неизменном виде внутри цикла? Или вы считаете, что вынос строк в отдельные именованные методы даст лучшее понимание их значения? И если так - то как бы вы назвали эти методы?

Обновляем Золотую сборку и тесты

В зависимости от ответа получаем необходимость изменять код. Время использовать Золотую сборку и писать тесты, которые будут запускаться и сравнивать результат выполнения изменённого кода с эталоном.

class GoldenMasterTest extends PHPUnit_Framework_TestCase {

    private $gmPath;

    function setUp() {
        $this->gmPath = __DIR__ . '/gm.txt';
    }

    function testGenerateOutput() {
        $times = 20000;
        $this->generateMany($times, $this->gmPath);
    }

    function testOutputMatchesGoldenMaster() {
        $times = 20000;
        $actualPath = '/tmp/actual.txt';
        $this->generateMany($times, $actualPath);
        $file_content_gm = file_get_contents($this->gmPath);
        $file_content_actual = file_get_contents($actualPath);
        $this->assertTrue($file_content_gm == $file_content_actual);
    }

    private function generateMany($times, $fileName) {...}

    private function generateOutput($seed) {...}
}

Мы создали другой тест, который сравнивает результаты выполнения оригинального кода и изменённого, проверив, что метод testGenerateOutput() генерирует результат только один раз, и больше ничего не делает. Также мы переместили файл с результатом работы золотой сборки "gm.txt" в текущую директорию, так как "/tmp" может очищаться при перезагрузке системы. Мы все ещё можем использовать эту директорию для размещения текущих результатов работы. В большинстве UNIX-систем "/tmp" размещается в оперативной памяти, что намного быстрее файловой системы. Если вы всё сделали правильно - тесты должны проходить без проблем.

Важно не забывать помечать сгенерированные тесты как “пропущенные” для предстоящих изменений. Если же вам больше по душе закомментировать или удалить тест целиком - можно поступить и так. Важно, чтобы Золотая сборка не изменялась в процессе преобразования кода. Она была сгенерирована однажды, и мы не хотим, чтобы она была кем-то подменена, чтобы быть уверенными, что мы всегда сравниваем результаты работы изменённого кода с оригиналом. Для большей уверенности можно создать резервную копию.

function testGenerateOutput() {
    $this->markTestSkipped();
    $times = 20000;
    $this->generateMany($times, $this->gmPath);
}

Я помечу тест как “пропущенный”. Наш тест будет помечен жёлтым значком, что означает, что тесты прошли, но некоторые из них пропущены или помечены незавершёнными.

Делаем первое изменение

Написав тесты, можно начать изменять код. По моему мнению, все строки можно хранить внутри цикла for. Мы переместим код из метода createRockQuestion() в цикл for, а метод удалим. Такой рефакторинг называется Встроенный Метод.

“Поместите содержимое метода в места его вызова и удалите метод” - Мартин Фаулер

Существует определенный набор шагов для проведения этого типа рефакторинга, как описано Мартином Фаулером в Рефакторинг. Улучшение существующего кода:

  • Проверить, что метод не является полиморфным
  • Найти все места вызова метода
  • Заменить все вызовы телом метода
  • Скомпилировать и протестировать
  • Удалить объявление метода

У нас нет подклассов, наследующихся от класса Game, так что первый шаг удовлетворён.

У нас есть только одно место вызова метода, внутри цикла for:

function  __construct() {

    $this->players = array();
    $this->places = array(0);
    $this->purses = array(0);
    $this->inPenaltyBox = array(0);

    $this->popQuestions = array();
    $this->scienceQuestions = array();
    $this->sportsQuestions = array();
    $this->rockQuestions = array();

    for ($i = 0; $i < 50; $i++) {
        array_push($this->popQuestions, "Pop Question " . $i);
        array_push($this->scienceQuestions, ("Science Question " . $i));
        array_push($this->sportsQuestions, ("Sports Question " . $i));
        array_push($this->rockQuestions, "Rock Question " . $i);
    }
}

function createRockQuestion($index) {
    return "Rock Question " . $index;
}

Мы поместили код из метода createRockQuestion() в цикл for в конструкторе. Старый код мы ещё не удалили. Настало время запустить тест.

Тесты проходят. Теперь можно удалить метод createRockQuestion().

function  __construct() {

    // ... //

    for ($i = 0; $i < 50; $i++) {
        array_push($this->popQuestions, "Pop Question " . $i);
        array_push($this->scienceQuestions, ("Science Question " . $i));
        array_push($this->sportsQuestions, ("Sports Question " . $i));
        array_push($this->rockQuestions, "Rock Question " . $i);
    }
}

function isPlayable() {
    return ($this->howManyPlayers() >= 2);
}

Теперь также стоит запустить тесты. Если мы где-то не заменили вызов метода - тесты не пройдут. В нашем случае тесты должны пройти. Поздравляю! Мы закончили проводить наше первый рефакторинг.

Другие строки, которые следует принять во внимание

Строки в методах add() и roll() используются только для вывода с помощью метода echoln(). Метод askQuestions() соотносит строки с категориями. Это также можно принять. С другой стороны, метод currentCategory() возвращает строку, зависящую от числа. В этом методе присутствует множество дублированных строк. Изменение любой категории, кроме категории “Rock” потребует её изменения в трёх местах, и это только в этом методе.

function currentCategory() {
    if ($this->places[$this->currentPlayer] == 0) {
        return "Pop";
    }
    if ($this->places[$this->currentPlayer] == 4) {
        return "Pop";
    }
    if ($this->places[$this->currentPlayer] == 8) {
        return "Pop";
    }
    if ($this->places[$this->currentPlayer] == 1) {
        return "Science";
    }
    if ($this->places[$this->currentPlayer] == 5) {
        return "Science";
    }
    if ($this->places[$this->currentPlayer] == 9) {
        return "Science";
    }
    if ($this->places[$this->currentPlayer] == 2) {
        return "Sports";
    }
    if ($this->places[$this->currentPlayer] == 6) {
        return "Sports";
    }
    if ($this->places[$this->currentPlayer] == 10) {
        return "Sports";
    }
    return "Rock";
}

Думаю, можно сделать лучше. Можно использовать рефакторинг под названием Ввод локальной переменной, и избежать дублирования. Следуем данным пунктам:

  • Добавьте переменную с нужным значением
  • Найдите все места использования значения
  • Замените все использования соответствующей переменной
function currentCategory() {
    $popCategory = "Pop";
    $scienceCategory = "Science";
    $sportCategory = "Sports";
    $rockCategory = "Rock";

    if ($this->places[$this->currentPlayer] == 0) {
        return $popCategory;
    }
    if ($this->places[$this->currentPlayer] == 4) {
        return $popCategory;
    }
    if ($this->places[$this->currentPlayer] == 8) {
        return $popCategory;
    }
    if ($this->places[$this->currentPlayer] == 1) {
        return $scienceCategory;
    }
    if ($this->places[$this->currentPlayer] == 5) {
        return $scienceCategory;
    }
    if ($this->places[$this->currentPlayer] == 9) {
        return $scienceCategory;
    }
    if ($this->places[$this->currentPlayer] == 2) {
        return $sportCategory;
    }
    if ($this->places[$this->currentPlayer] == 6) {
        return $sportCategory;
    }
    if ($this->places[$this->currentPlayer] == 10) {
        return $sportCategory;
    }
    return $rockCategory;
}

Это намного лучше. Возможно, вы уже подметили возможные улучшения, которые можно внести в код, но мы находимся только на самом начальном этапе работы. Руки так и чешутся изменить всё, что только можно, но прошу вас сдерживать ваши порывы. Во многих случая, пока код не до конца понят, поспешные изменения могут привести в тупик, или даже к поломанному коду. Если вы думаете, что есть вероятность забыть вашу мысль - просто запишите её как заметку, или создайте задачу в системе управления проектами. Мы же продолжим с проблемами строк.

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

Магические константы

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

include_once __DIR__ . '/Game.php';

$notAWinner;

$aGame = new Game();

$aGame->add("Chet");
$aGame->add("Pat");
$aGame->add("Sue");

do {

    $aGame->roll(rand(0, 5) + 1);

    if (rand(0, 9) == 7) {
        $notAWinner = $aGame->wrongAnswer();
    } else {
        $notAWinner = $aGame->wasCorrectlyAnswered();
    }

} while ($notAWinner);

Начнём с файла GameRunner.php, и нашей первой жертвой станет метод roll(), который возвращает случайные числа. Предыдущий автор не позаботился о том, чтобы дать им понятные имена. А мы сможем? Если проанализировать код:

rand(0, 5) + 1

Он возвратит число между единицей и шестёркой. Эта часть возвращает случайное число между нулём и пятью, к которому всегда прибавляется единица. Так что можно утверждать, что на выходе у нас всегда будет число между единицей и шестю. Теперь нужно рассмотреть контекст приложения. Мы разрабатываем игру trivia. Мы знаем, что у нас есть какое-то подобие доски, по которой должны передвигаться игроки. А чтобы это сделать - нужно бросить кости. У кости есть шесть граней, и она может выдавать числа между единицей и шестью. Звучит разумно.

$dice = rand(0, 5) + 1;
$aGame->roll($dice);

Это нормально? Снова используем рефакторинг Введения локальной переменной. Нашу переменную назовём $dice, и она будет представлять собой случайное число между единицей и шестью. Благодаря этому следующее выражение будет звучать более натурально: Игра, брось кость.

Запустили ли вы тесты? Я не упоминал о необходимости запуска тестов, но нам нужно их запускать так часто, как это возможно. И они должны проходить.

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

if (rand(0, 9) == 7)

Здесь уже чуть сложнее. Что в этом выражении означают ноль, девятка и семёрка? Может, их можно поименовать? На первый взгляд, я не могу сказать, что означают ноль и девятка, так что попробуем разобраться с семёркой. Если число, возвращаемое функцией генерации случайного числа, равно семи, будет выполнено условие под оператором if, что выдаёт неверный ответ. Так что, возможно, эту семёрку можно назвать $wrongAnswerId.

$wrongAnswerId = 7;
if (rand(0, 9) == $wrongAnswerId) {
    $notAWinner = $aGame->wrongAnswer();
} else {
    $notAWinner = $aGame->wasCorrectlyAnswered();
}

Наши тесты всё ещё проходят, а код стал немного выразительнее. Теперь, когда мы дали имя числу семь, выражение приобретает некий смысл. Можно подумать и о правильных именах и для нуля с девяткой. Они являются всего-лишь параметрами функции rand(), так что переменные можно именовать в стиле “минимальное нечто” и “максимальное нечто”.

$minAnswerId = 0;
$maxAnswerId = 9;
$wrongAnswerId = 7;
if (rand($minAnswerId, $maxAnswerId) == $wrongAnswerId) {
    $notAWinner = $aGame->wrongAnswer();
} else {
    $notAWinner = $aGame->wasCorrectlyAnswered();
}

Вот теперь проглядывается смысл. У нас есть минимальный идентификатор ответа, максимальный идентификатор ответа, и идентификатор неверного ответа. Загадка разгадана.

do {

    $dice = rand(0, 5) + 1;
    $aGame->roll($dice);

    $minAnswerId = 0;
    $maxAnswerId = 9;
    $wrongAnswerId = 7;
    if (rand($minAnswerId, $maxAnswerId) == $wrongAnswerId) {
        $notAWinner = $aGame->wrongAnswer();
    } else {
        $notAWinner = $aGame->wasCorrectlyAnswered();
    }

} while ($notAWinner);

Обратите внимание на весь код в цикле do-while. Надо ли нам каждый раз переписывать значения идентификаторов ответов? Думаю, нет. Давайте попробуем вынести определение переменных за цикл, и посмотреть, проходят ли наши тесты.

$minAnswerId = 0;
$maxAnswerId = 9;
$wrongAnswerId = 7;
do {
    $dice = rand(0, 5) + 1;
    $aGame->roll($dice);

    if (rand($minAnswerId, $maxAnswerId) == $wrongAnswerId) {
        $notAWinner = $aGame->wrongAnswer();
    } else {
        $notAWinner = $aGame->wasCorrectlyAnswered();
    }

} while ($notAWinner);

Да, тесты проходят, как и всегда.

Пора переключиться к файлу Game.php, проанализировать магические константы и там. Если у вас включена подсветка синтаксиса, то вы точно увидите, что константы выделены каким-то светлым цветом. У меня они синего цвета, и их легко отследить.

В этом цикле легко найти магическую константу 50. А если посмотреть, что делает код внутри цикла, можно заметить, что он заносит элементы в разные массивы. То есть, у нас есть какие-то списки, каждый с 50 элементами. Каждый список представляет собой категорию вопросов, а сами переменные объявлены свойствами класса, как массивы.

$this->popQuestions = array();
$this->scienceQuestions = array();
$this->sportsQuestions = array();
$this->rockQuestions = array();

Итак, что может представлять собой число 50? Уверен, что у вас уже появились какие-то мысли на этот счёт. Именование - одна из самых сложных вещей в программировании, если у вас возникло больше одной идеи, и вы не знаете, какую выбрать. Не стоит этого стыдиться. В моей голове также крутятся несколько вариантов именования, и я пытаюсь выбрать наиболее точно подходящее, даже во время написания этого параграфа. Думаю, можно подобрать консервативное имя данному параметру. Что-то, связанное с размером, вроде $questionsInEachCategory или $categorySize, или что-то близкое к этому.

$categorySize = 50;
for ($i = 0; $i < $categorySize; $i++) {
    array_push($this->popQuestions, "Pop Question " . $i);
    array_push($this->scienceQuestions, ("Science Question " . $i));
    array_push($this->sportsQuestions, ("Sports Question " . $i));
    array_push($this->rockQuestions, "Rock Question " . $i);
}

Смотрится прилично, можно оставить. И само собой разумеется, тесты проходят.

function isPlayable() {
    return ($this->howManyPlayers() >= 2);
}

Что означает эта двойка? Уверен, на этот момент вам ясен ответ. Это просто:

function isPlayable() {
    $minimumNumberOfPlayers = 2;
    return ($this->howManyPlayers() >= $minimumNumberOfPlayers);
}

Вы согласны? Если у вас есть лучший вариант - отписывайтесь ниже. А тесты? Они всё ещё проходят?

В методе roll() у нас есть ещё некоторые числа: два, ноль, одиннадцать и двенадцать.

if ($roll % 2 != 0)

Ну, это понятно. Вынесем это выражение в метод, но не в этом уроке. Мы всё ещё находимся в фазе понимания и охоты за магическими константами и строками. Так что такое 11 и 12? Они закопаны в глубинах третьего уровня if. Довольно непросто понять, что же они собой представляют. Может, стоит осмотреть окружающие их строки?

if ($this->places[$this->currentPlayer] > 11) {
    $this->places[$this->currentPlayer] = $this->places[$this->currentPlayer] - 12;
}

Если позиция текущего игрока больше 11, то его позиция будет уменьшена до текущей за вычетом 12. Похоже на случай, когда игрок доходит до конца доски или игрового поля, и его перемещают на изначальную позицию. Возможно в нулевую. Или, если наша игровая доска идёт по кругу - его поместят в соответствующую позицию от начала круга. Так что 11 может быть размером доски.

$boardSize = 11;
if ($this->inPenaltyBox[$this->currentPlayer]) {
    if ($roll % 2 != 0) {
        // ... //
        if ($this->places[$this->currentPlayer] > $boardSize) {
            $this->places[$this->currentPlayer] = $this->places[$this->currentPlayer] - 12;
        }
        // ... //
    } else {
        // ... //
    }
} else {
    // ... //
    if ($this->places[$this->currentPlayer] > $boardSize) {
        $this->places[$this->currentPlayer] = $this->places[$this->currentPlayer] - 12;
    }
// ... //
}

Не забудьте поменять 11 в обеих частях метода. Это позволит вынести переменную за пределы оператора if, на первый уровень отступов.

Но если 11 - это размер доски, то что тогда - 12? Мы вычитаем 12 из текущей позиции игрока, не 11. И почему бы нам просто не обнулить позицию игрока? Потому, что тесты не пройдут. Наша предыдущая догадка о том, что игрока перемещают в нулевую позицию при достижении границ доски, ошибочна. Скажем, игрок находится на позиции 10, и ему выпадает 4. 14 больше 11, так что в этом случае вступает в силу вычитание. Игрок окажется на позиции 10+4-12=2.

$lastPositionOnTheBoard

Это приводит нас к ещё одному возможному именованию для 11 и 12. Я думаю, что более правильно называть 12 $boardSize (размер доски). А что тогда остаётся на долю 11? Может, $lastPositionOnTheBoard? Немного длинно, но, по крайней мере, оно сообщает истинное значение магической константы.

$lastPositionOnTheBoard = 11;
$boardSize = 12;
if ($this->inPenaltyBox[$this->currentPlayer]) {
    if ($roll % 2 != 0) {
        // ... //
        if ($this->places[$this->currentPlayer] > $lastPositionOnTheBoard) {
            $this->places[$this->currentPlayer] = $this->places[$this->currentPlayer] - $boardSize;
        }
        // ... //
    } else {
        // ... //
    }
} else {
    // ... //
    if ($this->places[$this->currentPlayer] > $lastPositionOnTheBoard) {
        $this->places[$this->currentPlayer] = $this->places[$this->currentPlayer] - $boardSize;
    }
// ... //
}

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

Заключение

Я оставил в коде последнюю магическую константу. Найдёте ли вы её? Если вы посмотрите в итоговый код - то там она заменена. Но это жульничество. Удачи в поисках, и спасибо за внимание.

Данный урок подготовлен для вас командой сайта ruseller.com
Источник урока: http://code.tutsplus.com/tutorials/refactoring-legacy-code-part-2-magic-strings-constants--cms-20527
Перевел: Станислав Протасевич
Урок создан: 19 Ноября 2014
Просмотров: 6336
Правила перепечатки


5 последних уроков рубрики "PHP"

  • Фильтрация данных с помощью zend-filter

    Когда речь идёт о безопасности веб-сайта, то фраза "фильтруйте всё, экранируйте всё" всегда будет актуальна. Сегодня поговорим о фильтрации данных.

  • Контекстное экранирование с помощью zend-escaper

    Обеспечение безопасности веб-сайта — это не только защита от SQL инъекций, но и протекция от межсайтового скриптинга (XSS), межсайтовой подделки запросов (CSRF) и от других видов атак. В частности, вам нужно очень осторожно подходить к формированию HTML, CSS и JavaScript кода.

  • Подключение Zend модулей к Expressive

    Expressive 2 поддерживает возможность подключения других ZF компонент по специальной схеме. Не всем нравится данное решение. В этой статье мы расскажем как улучшили процесс подключение нескольких модулей.

  • Совет: отправка информации в Google Analytics через API

    Предположим, что вам необходимо отправить какую-то информацию в Google Analytics из серверного скрипта. Как это сделать. Ответ в этой заметке.

  • Подборка PHP песочниц

    Подборка из нескольких видов PHP песочниц. На некоторых вы в режиме online сможете потестить свой код, но есть так же решения, которые можно внедрить на свой сайт.

или авторизуйтесь, чтобы добавлять комментарии, оценивать уроки и сохранять их в личном кабинете
  • 20 Ноября 2014 16:27
    xzoner
    Сделайте пожалуйста урок по настройке XDebug в PHPStorm. За статью спасибо!
^ Наверх ^