В нашем предыдущем уроке мы узнали новый способ понять и улучшить код, извлекая его до тех пор, пока мы не уйдем. Хотя это руководство было хорошим способом изучения техник, оно едва ли было идеальным примером, чтобы понять его преимущества. В этом уроке мы будем извлекать, пока не разберемся со всем кодом, связанным с викторинами, и проанализируем конечный результат.
Этот урок также завершит нашу серию о рефакторинге. Если вы считаете, что мы что-то упустили, не стесняйтесь комментировать предложенную тему. Если хорошие идеи соберутся, я продолжу дополнительные уроки на основе ваших запросов.
Нападение на наш самый длинный метод
Что может быть лучше для начала нашей статьи, чем взять наш самый длинный метод и извлечь его в крошечные кусочки. Первое тестирование, как обычно, сделает эту процедуру не только эффективной, но и увлекательной.
Как обычно, у вас есть код, который был, когда я запускал этот урок в папке php_start
, а конечный результат — в папке php
.
функция была Правильно ответили () { if ($ this-> inPenaltyBox [$ this-> currentPlayer]) { if ($ this-> isGettingOutOfPenaltyBox) { $ This-> Экран-> CorrectAnswer (); $ This-> кошельками [$ this-> currentPlayer] ++; $ this-> display-> playerCoins ($ this-> Players [$ this-> currentPlayer], $ this-> кошельки [$ this-> currentPlayer]); $ winner = $ this-> didPlayerNotWin (); $ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) { $ this-> currentPlayer = 0; } вернуть $ победителя; } еще { $ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) { $ this-> currentPlayer = 0; } вернуть истину; } } еще { $ This-> Экран-> correctAnswerWithTypo (); $ This-> кошельками [$ this-> currentPlayer] ++; $ this-> display-> playerCoins ($ this-> Players [$ this-> currentPlayer], $ this-> кошельки [$ this-> currentPlayer]); $ winner = $ this-> didPlayerNotWin (); $ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) { $ this-> currentPlayer = 0; } вернуть $ победителя; } }
Этот метод wasCorrectlyAnswered()
является нашей первой жертвой.
Получение wasCorrectlyAnspted () под тестом
Как мы узнали из предыдущих уроков, первый шаг при изменении устаревшего кода — это его тестирование. Это может быть сложным процессом. К счастью для нас, метод wasCorrectlyAnswered()
довольно прост. Он состоит из нескольких операторов if-else
. Каждая ветвь кода возвращает значение. Когда у нас есть возвращаемое значение, мы всегда можем предположить, что тестирование выполнимо. Не обязательно просто, но по крайней мере возможно.
function testWasCorrectlyAnsptedAndGettingOutOfPenaltyBoxWhileBeingAWinner () { $ This-> setAPlayerThatIsInThePenaltyBox (); $ this-> game-> isGettingOutOfPenaltyBox = true; $ this-> game-> кошельки [$ this-> game-> currentPlayer] = Game :: $ numberOfCoinsToWin; $ This-> assertTrue ($ this-> игры-> wasCorrectlyAnswered ()); }
Не существует определенного правила о том, какой тест писать первым. Мы только что выбрали первый путь исполнения здесь. На самом деле у нас был приятный сюрприз, и мы повторно использовали один из частных методов, которые мы извлекли много уроков раньше. Но мы еще не закончили. Все зеленые, так что пришло время для рефакторинга.
function testWasCorrectlyAnsptedAndGettingOutOfPenaltyBoxWhileBeingAWinner () { $ This-> setAPlayerThatIsInThePenaltyBox (); $ This-> currentPlayerWillLeavePenaltyBox (); $ This-> setCurrentPlayerAWinner (); $ This-> assertTrue ($ this-> игры-> wasCorrectlyAnswered ()); }
Это легче читать и значительно более наглядно. Вы можете найти извлеченные методы в прикрепленном коде.
function testWasCorrectlyAnsptedAndGettingOutOfPenaltyBoxWhileNOTBeingAWinner () { $ This-> setAPlayerThatIsInThePenaltyBox (); $ This-> currentPlayerWillLeavePenaltyBox (); $ This-> setCurrentPlayerNotAWinner (); $ This-> assertFalse ($ this-> игры-> wasCorrectlyAnswered ()); } приватная функция setCurrentPlayerNotAWinner () { $ this-> game-> кошельки [$ this-> game-> currentPlayer] = 0; }
Мы ожидали, что это пройдет, но это не удалось. Причины не ясны вообще. Более внимательный взгляд на didPlayerNotWin()
может быть полезным.
function didPlayerNotWin () { return! ($ this-> кошельки [$ this-> currentPlayer] == self :: $ numberOfCoinsToWin); }
Метод на самом деле возвращает истину, когда игрок не выиграл. Возможно, мы могли бы переименовать нашу переменную, но сначала тесты должны пройти.
приватная функция setCurrentPlayerAWinner () { $ this-> game-> кошельки [$ this-> game-> currentPlayer] = Game :: $ numberOfCoinsToWin; } приватная функция setCurrentPlayerNotAWinner () { $ this-> game-> кошельки [$ this-> game-> currentPlayer] = 0; }
При ближайшем рассмотрении мы видим, что мы перепутали значения здесь. Наша путаница между именем метода и именем переменной заставила нас изменить условия.
приватная функция setCurrentPlayerAWinner () { $ this-> game-> кошельки [$ this-> game-> currentPlayer] = 0; } приватная функция setCurrentPlayerNotAWinner () { $ this-> game-> кошельки [$ this-> game-> currentPlayer] = Game :: $ numberOfCoinsToWin - 1; }
Это работает При анализе didPlayerNotWin()
мы также заметили, что он использует равенство для определения победителя. Мы должны установить наше значение на единицу меньше, потому что значение увеличивается в производственном коде, который мы тестируем.
Оставшиеся три теста просто написать. Это всего лишь вариации первых двух. Вы можете найти их в прикрепленном коде.
Извлечение и переименование до тех пор, пока мы не удалим метод wasCorrectlyAnspted ()
Наиболее запутанная проблема — вводящее в заблуждение имя переменной $winner
. Это должно быть $notAWinner
.
$ notAWinner = $ this-> didPlayerNotWin (); $ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) { $ this-> currentPlayer = 0; } вернуть $ notAWinner;
Мы можем заметить, что переменная $notAWinner
используется только для возврата значения. Можем ли мы вызвать метод didPlayerNotWin()
непосредственно в операторе возврата?
$ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) { $ this-> currentPlayer = 0; } return $ this-> didPlayerNotWin ();
Это все еще проходит наш модульный тест, но если мы запустим наши тесты Golden Master, они потерпят неудачу с ошибкой «недостаточно памяти». Фактически, изменение делает игру никогда не законченной.
Происходит то, что текущий игрок обновляется до следующего игрока. Поскольку у нас был один игрок, мы всегда использовали одного и того же игрока. Вот как проходит тестирование. Вы никогда не знаете, когда обнаружите скрытую логику в сложном коде.
function testWasCorrectlyAnsptedAndGettingOutOfPenaltyBoxWhileBeingAWinner () { $ This-> setAPlayerThatIsInThePenaltyBox (); $ this-> game-> add ('Another Player'); $ This-> currentPlayerWillLeavePenaltyBox (); $ This-> setCurrentPlayerAWinner (); $ This-> assertTrue ($ this-> игры-> wasCorrectlyAnswered ()); }
Просто добавив другого игрока в каждом из наших тестов, связанных с этим методом, мы можем убедиться, что логика покрыта. Этот тест приведет к тому, что измененный оператор возврата, приведенный выше, завершится неудачей.
приватная функция selectNextPlayer () { $ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) { $ this-> currentPlayer = 0; } }
Мы сразу можем заметить, что выбор следующего игрока идентичен на обоих путях условия. Мы можем переместить это в собственный метод. Имя, которое мы выбрали для этого метода — selectNextPlayer()
. Это имя помогает подчеркнуть тот факт, что значение текущего игрока будет изменено. Это также предполагает, что didPlayerNotWin()
может быть переименован во что-то, что отражает отношение к текущему игроку.
функция была Правильно ответили () { if ($ this-> inPenaltyBox [$ this-> currentPlayer]) { if ($ this-> isGettingOutOfPenaltyBox) { $ This-> Экран-> CorrectAnswer (); $ This-> кошельками [$ this-> currentPlayer] ++; $ this-> display-> playerCoins ($ this-> Players [$ this-> currentPlayer], $ this-> кошельки [$ this-> currentPlayer]); $ notAWinner = $ this-> didCurrentPlayerNotWin (); $ This-> selectNextPlayer (); вернуть $ notAWinner; } еще { $ This-> selectNextPlayer (); вернуть истину; } } еще { $ This-> Экран-> correctAnswerWithTypo (); $ This-> кошельками [$ this-> currentPlayer] ++; $ this-> display-> playerCoins ($ this-> Players [$ this-> currentPlayer], $ this-> кошельки [$ this-> currentPlayer]); $ notAWinner = $ this-> didCurrentPlayerNotWin (); $ This-> selectNextPlayer (); вернуть $ notAWinner; } }
Наш код становится короче и выразительнее. Что мы можем сделать дальше? Мы могли бы изменить странное имя логики «не победитель» и изменить метод на положительную логику вместо отрицательной. Или мы могли бы продолжить извлечение и разобраться с негативной логикой позже. Я не думаю, что есть один определенный путь. Итак, я оставлю проблему негативной логики в качестве упражнения для вас, и мы продолжим извлечение.
функция была Правильно ответили () { if ($ this-> inPenaltyBox [$ this-> currentPlayer]) { return $ this-> getCorrectlyAnsededForPlayersInPenaltyBox (); } еще { return $ this-> getCorrectlyAnsededForPlayersNotInPenaltyBox (); } }
Как правило, старайтесь иметь одну строку кода на каждом пути логики принятия решения.
Мы извлекли весь блок кода в каждой части нашего оператора if
. Это важный шаг и то, о чем вы всегда должны думать. Когда в вашем коде есть путь принятия решения или цикл, внутри него должен быть только один оператор. Человек, читающий этот метод, скорее всего, не будет заботиться о деталях реализации. Он или она будет заботиться о логике принятия решения, утверждении if
.
функция была Правильно ответили () { if ($ this-> inPenaltyBox [$ this-> currentPlayer]) { return $ this-> getCorrectlyAnsededForPlayersInPenaltyBox (); } return $ this-> getCorrectlyAnsptedForPlayersNotInPenaltyBox (); }
И если мы сможем избавиться от лишнего кода, мы должны. Удалив else
и сохранив логику, мы немного сэкономили. Мне больше нравится это решение, потому что оно подчеркивает поведение функции по умолчанию. Код, который находится непосредственно внутри функции (последняя строка кода здесь). Оператор if
является исключительной функциональностью, добавленной к стандартному.
Я слышал рассуждения о том, что написание условий таким образом может скрыть тот факт, что по умолчанию не выполняется, if
оператор if
активируется. Я могу только согласиться с этим, и если вы предпочитаете оставить else
часть для ясности, пожалуйста, сделайте это.
приватная функция getCorrectlyAnsptedForPlayersInPenaltyBox () { if ($ this-> isGettingOutOfPenaltyBox) { return $ this-> getCorrectlyAnsptedForPlayerGettingOutOfPenaltyBox (); } еще { return $ this-> getCorrectlyAnsptedForPlayerStayingInPenaltyBox (); } }
Мы можем продолжать извлекать внутри наших недавно созданных частных методов. Применение того же принципа к нашему следующему условному утверждению приводит к приведенному выше коду.
приватная функция giveCurrentUserACoin () { $ This-> кошельками [$ this-> currentPlayer] ++; }
Глядя на наши закрытые методы getCorrectlyAnsweredForPlayersNotInPenaltyBox()
и getCorrectlyAnsweredForPlayerGettingOutOfPenaltyBox()
мы сразу можем заметить, что простое назначение дублируется. Это назначение может быть очевидным для кого-то вроде нас, который уже знает, что с кошельками и монетами, но не для новичка. Извлечение этой единственной строки в метод giveCurrentUserACoin()
является хорошей идеей.
Это также помогает в дублировании. Если в будущем мы изменим способ выдачи игрокам монет, нам нужно будет изменить код только внутри этого закрытого метода.
приватная функция getCorrectlyAnsptedForPlayersNotInPenaltyBox () { $ This-> Экран-> correctAnswerWithTypo (); return $ this-> getCorrectlyAnsededForAPlayer (); } приватная функция getCorrectlyAnsptedForPlayerGettingOutOfPenaltyBox () { $ This-> Экран-> CorrectAnswer (); return $ this-> getCorrectlyAnsededForAPlayer (); } приватная функция getCorrectlyAnsptedForAPlayer () { $ This-> giveCurrentUserACoin (); $ this-> display-> playerCoins ($ this-> Players [$ this-> currentPlayer], $ this-> кошельки [$ this-> currentPlayer]); $ notAWinner = $ this-> didCurrentPlayerNotWin (); $ This-> selectNextPlayer (); вернуть $ notAWinner; }
Тогда два правильно отвеченных метода идентичны, за исключением того, что один из них выводит что-то с опечаткой. Мы извлекли дубликат кода и сохранили различия в каждом методе. Вы можете подумать, что мы могли бы использовать извлеченный метод с параметром в коде вызывающего и выводить один раз нормально и один раз с опечаткой. Однако решение, предложенное выше, имеет преимущество: оно разделяет две концепции: не в штрафной и выход из штрафной.
На этом работа над wasCorrectlyAnswered()
.
Как насчет метода worngAnswer ()?
function неправильный ответ () { $ This-> Экран-> incorrectAnswer (); $ currentPlayer = $ this-> Players [$ this-> currentPlayer]; $ This-> Экран-> playerSentToPenaltyBox ($ currentPlayer); $ this-> inPenaltyBox [$ this-> currentPlayer] = true; $ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) { $ this-> currentPlayer = 0; } вернуть истину; }
На 11 строчках этот метод не огромный, но, безусловно, большой. Вы помните магическое число семь плюс минус два исследования? В нем говорится, что наш мозг может одновременно думать о 7 + -2 вещах. То есть у нас ограниченные возможности. Поэтому, чтобы легко и полностью понять метод, мы хотим, чтобы логика внутри него вписывалась в этот диапазон. В общей сложности 11 строк и 9 строк, этот метод является своего рода пределом. Вы можете утверждать, что на самом деле есть пустая строка, а другая — только с фигурной скобкой. Это сделало бы только 7 строк логики.
Хотя фигурные скобки и пробелы короткие в пространстве, они имеют значение для нас. Они разделяют части логики, они имеют смысл, поэтому наш мозг должен их обрабатывать. Да, это проще по сравнению с полной строкой логики сравнения, но все же.
Вот почему наша цель для строк логики внутри метода — 4 строки. Это ниже минимума вышеупомянутой теории, поэтому и гений, и посредственный программист должны быть в состоянии понять метод.
$ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) { $ this-> currentPlayer = 0; }
У нас уже есть метод для этого куска кода, поэтому мы должны его использовать.
function неправильный ответ () { $ This-> Экран-> incorrectAnswer (); $ currentPlayer = $ this-> Players [$ this-> currentPlayer]; $ This-> Экран-> playerSentToPenaltyBox ($ currentPlayer); $ this-> inPenaltyBox [$ this-> currentPlayer] = true; $ This-> selectNextPlayer (); вернуть истину; }
Лучше, но мы должны отказаться или продолжить?
$ currentPlayer = $ this-> Players [$ this-> currentPlayer]; $ This-> Экран-> playerSentToPenaltyBox ($ currentPlayer);
Мы могли бы вставить переменную из этих двух строк. $this->currentPlayer
, очевидно, возвращает текущего игрока, поэтому не нужно повторять логику. Мы не узнаем ничего нового или ничего абстрактного с помощью локальной переменной.
function неправильный ответ () { $ This-> Экран-> incorrectAnswer (); $ This-> Экран-> playerSentToPenaltyBox ($ this-> игроки [$ this-> currentPlayer]); $ this-> inPenaltyBox [$ this-> currentPlayer] = true; $ This-> selectNextPlayer (); вернуть истину; }
Мы до 5 строк. Что-нибудь еще там?
$ this-> inPenaltyBox [$ this-> currentPlayer] = true;
Мы можем извлечь строку выше в свой собственный метод. Это поможет объяснить происходящее и изолировать логику отправки текущего игрока в штрафную на своем месте.
function неправильный ответ () { $ This-> Экран-> incorrectAnswer (); $ This-> Экран-> playerSentToPenaltyBox ($ this-> игроки [$ this-> currentPlayer]); $ This-> sendCurrentPlayerToPenaltyBox (); $ This-> selectNextPlayer (); вернуть истину; }
Еще 5 строк, но все вызовы метода. Первые два показывают вещи. Следующие два связаны с нашей логикой. Последняя строка просто возвращает истину. Я не вижу способа облегчить понимание этого метода, не внося сложности в те извлечения, которые мы могли бы сделать, например, путем извлечения двух методов отображения в частный. Если мы хотим сделать это, куда этот метод должен пойти? В этот Game
класс или в Display
? Я думаю, что это уже слишком сложный вопрос, чтобы его можно было рассмотреть в связи с явной простотой нашего метода.
Заключительные мысли и некоторые статистические данные
Давайте сделаем немного статистики, проверив этот замечательный инструмент от автора PHPUnit https://github.com/sebastianbergmann/phploc.git
Статистика по оригинальному игровому коду викторины
01
02
03
04
05
06
07
08
09
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
|
./phploc ../Refactoring\ Legacy\ Code\ -\ Part\ 1\:\ The\ Golden\ Master/Source/trivia/php/
phploc 2.1-gca70e70 by Sebastian Bergmann.
Size
Lines of Code (LOC) 232
Comment Lines of Code (CLOC) 0 (0.00%)
Non-Comment Lines of Code (NCLOC) 232 (100.00%)
Logical Lines of Code (LLOC) 99 (42.67%)
Classes 88 (88.89%)
Average Class Length 88
Minimum Class Length 88
Maximum Class Length 88
Average Method Length 7
Minimum Method Length 1
Maximum Method Length 17
Functions 1 (1.01%)
Average Function Length 1
Not in classes or functions 10 (10.10%)
Cyclomatic Complexity
Average Complexity per LLOC 0.26
Average Complexity per Class 25.00
Minimum Class Complexity 25.00
Maximum Class Complexity 25.00
Average Complexity per Method 3.18
Minimum Method Complexity 1.00
Maximum Method Complexity 10.00
Dependencies
Global Accesses 0
Global Constants 0 (0.00%)
Global Variables 0 (0.00%)
Super-Global Variables 0 (0.00%)
Attribute Accesses 115
Non-Static 115 (100.00%)
Static 0 (0.00%)
Method Calls 21
Non-Static 21 (100.00%)
Static 0 (0.00%)
Structure
Namespaces 0
Interfaces 0
Traits 0
Classes 1
Abstract Classes 0 (0.00%)
Concrete Classes 1 (100.00%)
Methods 11
Scope
Non-Static Methods 11 (100.00%)
Static Methods 0 (0.00%)
Visibility
Public Methods 11 (100.00%)
Non-Public Methods 0 (0.00%)
Functions 1
Named Functions 1 (100.00%)
Anonymous Functions 0 (0.00%)
Constants 0
Global Constants 0 (0.00%)
Class Constants 0 (0.00%)
|
Статистика по исправленному коду викторины
./phploc ../ Рефакторинг \ Legacy \ Code \ - \ Part \ 11 \: \ The \ End \? / Source / trivia / php phploc 2.1-gca70e70 от Себастьяна Бергмана. Размер Строки кода (LOC) 371 Строки кода комментария (CLOC) 0 (0,00%) Строки кода без комментариев (NCLOC) 371 (100,00%) Логические строки кода (LLOC) 151 (40,70%) Классы 145 (96,03%) Средняя длина класса 36 Минимальная длина класса 8 Максимальная длина класса 89 Средняя длина метода 2 Минимальная длина метода 1 Максимальная длина метода 14 Функции 0 (0,00%) Средняя длина функции 0 Не в классах или функциях 6 (3,97%) Цикломатическая Сложность Средняя сложность на LLOC 0,15 Средняя сложность на класс 6.50 Минимальная сложность класса 1,00 Максимальная сложность класса 17.00 Средняя сложность на метод 1.46 Минимальная сложность метода 1,00 Максимальная сложность метода 10,00 зависимости Глобальный доступ 0 Глобальные константы 0 (0,00%) Глобальные переменные 0 (0,00%) Суперглобальные переменные 0 (0,00%) Доступ к атрибутам 96 Нестатический 94 (97,92%) Статический 2 (2,08%) Вызовы метода 74 Нестатические 74 (100,00%) Статический 0 (0,00%) Структура Пространства имен 0 Интерфейсы 1 Черты 0 Занятия 3 Абстрактные классы 0 (0,00%) Бетонные классы 3 (100,00%) Методы 59 Сфера Нестатические методы 59 (100,00%) Статические методы 0 (0,00%) видимость Публичные методы 35 (59,32%) Непубличные методы 24 (40,68%) Функции 0 Именованные функции 0 (0,00%) Анонимные функции 0 (0,00%) Константы 3 Глобальные константы 0 (0,00%) Константы класса 3 (100,00%)
Анализ
Грубые данные настолько хороши, насколько мы можем их понять и проанализировать.
Количество логических строк кода значительно увеличилось с 99 до 151. Но это число не должно обманывать вас, заставляя думать, что наш код стал более сложным. Это естественная тенденция хорошо переработанного кода из-за увеличения количества методов и вызовов к ним.
Как только мы посмотрим на среднюю длину класса, мы увидим резкое снижение количества строк кода с 88 до 36.
И просто удивительно, как длина метода уменьшилась со среднего значения в семь строк до двух строк кода.
Хотя количество строк является хорошим индикатором объема кода на единицу измерения, реальный выигрыш заключается в анализе цикломатической сложности. Каждый раз, когда мы принимаем решение в нашем коде, мы увеличиваем цикломатическую сложность. Когда мы соединяем операторы if
внутри другого, цикломатическая сложность этого метода возрастает экспоненциально. Наши постоянные извлечения привели к методам, в которых принималось только одно решение, что снизило их среднюю сложность для метода с 3,18 до 1,00. Вы можете прочитать это как «наши рефакторинговые методы в 3,18 раза проще, чем исходный код». На уровне класса падение сложности еще более удивительно. Он снизился с 25.00 до 6.50.
Конец?
Что ж. Вот и все. Конец серии. Пожалуйста, не стесняйтесь высказывать свое мнение, и если вы считаете, что мы пропустили какую-либо тему рефакторинга, попросите их в комментариях ниже. Если они будут интересны, я трансформирую их в дополнительные части этой серии.
Спасибо за ваше пристальное внимание.