Рефакторинг унаследованного кода: Часть 2 - Магические строки и константы
Содержание цикла статей "Рефакторинг унаследованного кода":
Часть 1 - Золотой мастер
Часть 2 - Магические строки и константы
Часть 3 - Сложные условия
Часть 4 - Наш первый модульный тест
Старый код. Плохой код. Сложный код. Запутанный код. Полная ерунда. Два слова – унаследованный код. Это серия статей поможет вам работать с ним.
В предыдущей статье мы взялись за модернизацию унаследованного исходного кода. Мы запустили код на исполнение, чтобы сформировать суждение о его базовой функциональности, и создали набор тестов «Золотой мастер», чтобы заложить основу для обеспечения безопасности кода при последующем внесении изменений.
Мы продолжим работу над этим кодом, который вы можете найти в папке 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;
}Итак, давайте проанализируем строки с 32-ой по 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() возвращает строки, исходя из количества.
В этом методе существует много дублированных строк. Изменение любой категории, кроме категории «Рок», повлечет необходимость изменить его имя в трех местах, только в этом методе:
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он возвращает число от одного до шести. Блок генерации случайных чисел возвращает число от нуля до пяти, к которому всегда добавляется единица. Так что, в конечном итоге, получается, от одного до шести. Теперь нам нужно рассмотреть контекст нашего приложения.
Мы разрабатываем настольную викторину. Мы знаем, что существует определенного рода доска, по которой перемещаются наши участники. Чтобы сделать ход, мы должны бросить кости. Кубик имеет шесть граней, и в результате выбрасывания может выпасть цифра от одного до шести. Кажется, мы вычислили кое-какую логику:
$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 в цикле for было довольно легко. И если мы посмотрим, что делает этот код, мы можем обнаружить, что внутри цикла for элементы распределяются в несколько массивов. И у нас есть некоторые списки, каждый из которых состоит из 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() у нас также есть некоторые числа: 2, 0, 11 и 12:
if ($roll % 2 != 0)Насчет 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. Почему бы нам просто не установить позицию на ноль, вместо того чтобы вычитать что-то? Да потому что тогда наши тесты не пройдут.
Наше предыдущее предположение о том, что игрок окажется в начальной позиции ноль, после того, как будет исполнен код внутри оператора if, оказалось ошибочным. Давайте предположим, что игрок, находящийся на позиции десять, выбрасывает четыре. 14 больше, чем 11, поэтому производится вычитание. Игрок окажется на позиции 10+4-12 = 2.
Это дает нам новую гипотезу насчет того, как можно назвать переменные для чисел 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;
}
// ... //
}Знаю, знаю! Здесь присутствует дублированный код. Вполне очевидный, особенно с учетом остальной части кода, которая тут не видна. Но помните, что мы продолжим работу с кодом после поиска магических констант. Тогда у нас будет возможность заняться дублированным кодом, но не сейчас.
Заключение
Я оставил в коде еще одну, последнюю магическую константу. Сможете найти ее сами? Если вы заглянете в окончательный вариант кода, то там она уже заменена, но это, конечно, будет нечестно с вашей стороны. Удачи в ее поисках и спасибо, что уделили мне внимание.