Создание, анализ ирефакторинг
 Скачать 3.16 Mb.
|
|
if (expected == null || actual == null || areStringsEqual()) return Assert.format(message, expected, actual); findCommonPrefix(); findCommonSuffix(); String expected = compactString(this.expected); String actual = compactString(this.actual); return Assert.format(message, expected, actual); } 1 См . раздел «Правило бойскаута» на с . 37 . 293 294 Глава 15 . Внутреннее строение JUnit Инкапсуляция поможет лучше выразить намерения разработчика . Поэтому я создал метод с именем, поясняющим его смысл: public String compact(String message) { if ( shouldNotCompact()) return Assert.format(message, expected, actual); findCommonPrefix(); findCommonSuffix(); String expected = compactString(this.expected); String actual = compactString(this.actual); return Assert.format(message, expected, actual); } private boolean shouldNotCompact() { return expected == null || actual == null || areStringsEqual(); } Запись this.expected и this.actual в функции compact тоже оставляет желать лучшего . Это произошло, когда мы переименовали fExpected в expected . Зачем в функции используются переменные с именами, совпадающими с именами пере- менных класса? Ведь они имеют разный смысл [N4]? Неоднозначность в именах следует исключить . String compactExpected = compactString(expected); String compactActual = compactString(actual); Отрицательные условия чуть сложнее для понимания, чем положительные [G29] . Чтобы проверяемое условие стало более понятным, мы инвертируем его: public String compact(String message) { if ( canBeCompacted()) { findCommonPrefix(); findCommonSuffix(); String compactExpected = compactString(expected); String compactActual = compactString(actual); return Assert.format(message, compactExpected, compactActual); } else { return Assert.format(message, expected, actual); } } private boolean canBeCompacted() { return expected != null && actual != null && !areStringsEqual(); } Имя функции compact выглядит немного странно [N7] . Хотя она выполняет сжа- тие строк, этого не произойдет, если canBeCompacted вернет false . Таким образом, выбор имени compact скрывает побочный эффект проверки . Также обратите внимание на то, что функция возвращает отформатированное сообщение, а не просто сжатые строки . Следовательно, функцию было бы правильнее назвать formatCompactedComparison . В этом случае она гораздо лучше читается вместе с аргументом: 294 Инфраструктура JUnit 295 public String formatCompactedComparison(String message) { Тело команды if — то место, где выполняется фактическое сжатие строк expected и actual . Мы извлечем этот код в метод compactExpectedAndActual . Тем не менее все форматирование должно происходить в функции formatCompactedComparison Функция compact . . . не должна делать ничего, кроме сжатия [G30] . Разобьем ее следующим образом: private String compactExpected; private String compactActual; public String formatCompactedComparison(String message) { if (canBeCompacted()) { compactExpectedAndActual(); return Assert.format(message, compactExpected, compactActual); } else { return Assert.format(message, expected, actual); } } private void compactExpectedAndActual() { findCommonPrefix(); findCommonSuffix(); compactExpected = compactString(expected); compactActual = compactString(actual); } Обратите внимание: это преобразование заставило нас повысить compactExpected и compactActual до переменных класса . Еще мне не нравится то, что в двух послед- них строках новой функции возвращаются переменные, а в первых двух — нет . Это противоречит рекомендациям по использованию единых конвенций [G11] . Значит, функции findCommonPrefix и findCommon Suffix следует изменить так, чтобы они возвращали значения префикса и суффикса . private void compactExpectedAndActual() { prefixIndex_))_break;_}_return_prefixIndex;}295_296__Глава_15_.'>Index = findCommonPrefix(); suffixIndex = findCommonSuffix(); compactExpected = compactString(expected); compactActual = compactString(actual); } private int findCommonPrefix() { int prefixIndex = 0; int end = Math.min(expected.length(), actual.length()); for (; prefix Index < end; prefixIndex++) { if (expected.charAt(prefix Index) != actual.charAt(prefixIndex)) break; } return prefixIndex; } 295 296 Глава 15 . Внутреннее строение JUnit private int findCommonSuffix() { int expectedSuffix = expected.length() - 1; int actualSuffix = actual.length() - 1; for (; actualSuffix >= prefix Index && expectedSuffix >= prefixIndex; actualSuffix--, expectedSuffix--) { if (expected.charAt(expectedSuffix) != actual.charAt(actualSuffix)) break; } return expected.length() - expectedSuffix; } Также следует изменить имена переменных класса так, чтобы они стали чуть более точными [N1]; в конце концов, обе переменные представляют собой ин- дексы . Тщательное изучение findCommonSuffix выявляет скрытую временную привязку [G31]; работа функции зависит от того, что значение prefixIndex вычисляется функцией findCommonPrefix . Если вызвать эти две функции в неверном порядке, вам предстоит непростой сеанс отладки . Чтобы эта временная привязка стала очевидной, значение prefixIndex будет передаваться при вызове findCommonSuffix в аргументе . private void compactExpectedAndActual() { prefixIndex = findCommonPrefix(); suffixIndex = findCommonSuffix( prefixIndex); compactExpected = compactString(expected); compactActual = compactString(actual); } private int findCommonSuffix( int prefixIndex) { int expectedSuffix = expected.length() - 1; int actualSuffix = actual.length() - 1; for (; actualSuffix >= prefixIndex && expectedSuffix >= prefixIndex; actualSuffix--, expectedSuffix--) { if (expected.charAt(expectedSuffix) != actual.charAt(actualSuffix)) break; } return expected.length() - expectedSuffix; } Но и такое решение оставляет желать лучшего . Передача аргумента prefixIndex выглядит нелогично [G32] . Она устанавливает порядок вызова, но никоим об- разом не объясняет необходимость именно такого порядка . Другой программист может отменить внесенное изменение, так как ничто не указывает на то, что этот параметр действительно необходим . private void compactExpectedAndActual() { findCommonPrefixAndSuffix(); compactExpected = compactString(expected); compactActual = compactString(actual); } private void findCommonPrefixAndSuffix() { 296 Инфраструктура JUnit 297 findCommonPrefix(); int expectedSuffix = expected.length() - 1; int actualSuffix = actual.length() - 1; for (; actualSuffix >= prefixIndex && expectedSuffix >= prefixIndex; actualSuffix--, expectedSuffix-- ) { if (expected.charAt(expectedSuffix) != actual.charAt(actualSuffix)) break; } suffixIndex = expected.length() - expectedSuffix; } private void findCommonPrefix() { prefixIndex = 0; int end = Math.min(expected.length(), actual.length()); for (; prefixIndex < end; prefixIndex++) if (expected.charAt(prefixIndex) != actual.charAt(prefixIndex)) break; } Функции findCommonPrefix и findCommonSuffix возвращаются к прежнему виду, функция findCommonSuffix переименовывается в findCommonPrefixAndSuffix , и в нее включается вызов findCommonPrefix до выполнения каких-либо других действий . Тем самым временная связь двух функций устанавливается гораздо более ради- кально, чем в предыдущем решении . Кроме того, новое решение со всей очевид- ностью демонстрирует, насколько уродлива функция findCommonPrefixAndSuffix Давайте немного почистим ее . private void findCommonPrefixAndSuffix() { findCommonPrefix(); int suffixLength = 1; for (; !suffixOverlapsPrefix(suffixLength); suffixLength++) { if (charFromEnd(expected, suffixLength) != charFromEnd(actual, suffixLength)) break; } suffixIndex = suffixLength; } private char charFromEnd(String s, int i) { return s.charAt(s.length()-i);} private boolean suffixOverlapsPrefix(int suffixLength) { return actual.length() - suffixLength < prefixLength || expected.length() - suffixLength < prefixLength; } Так гораздо лучше . Новая версия кода очевидно показывает, что suffixIndex в действительности определяет длину суффикса, а прежнее имя было выбрано неудачно . Это относится и к prefixIndex , хотя в данном случае «индекс» и «дли- на» являются синонимами . Несмотря на это, использование термина «длина» 297 298 Глава 15 . Внутреннее строение JUnit выглядит более последовательно . Проблема в том, что значение переменной suf- fixIndex отсчитывается не от 0, а от 1, так что называть его «длиной» не совсем коррект но (кстати, этим же обстоятельством объясняются загадочные прибавле- ния +1 в computeCommonSuffix [G33]) . Давайте исправим этот недостаток . Результат показан в листинге 15 .4 . листинг 15 .4 . ComparisonCompactor.java (промежуточная версия) public class ComparisonCompactor { private int suffixLength; private void findCommonPrefixAndSuffix() { findCommonPrefix(); suffixLength = 0; for (; !suffixOverlapsPrefix(suffixLength); suffixLength++) { if (charFromEnd(expected, suffixLength) != charFromEnd(actual, suffixLength)) break; } } private char charFromEnd(String s, int i) { return s.charAt(s.length() - i - 1); } private boolean suffixOverlapsPrefix(int suffixLength) { return actual.length() - suffixLength <= prefixLength || expected.length() - suffixLength <= prefixLength; } private String compactString(String source) { String result = DELTA_START + source.substring(prefixLength, source.length() - suffixLength) + DELTA_END; if (prefixLength > 0) result = computeCommonPrefix() + result; if ( suffixLength > 0) result = result + computeCommonSuffix(); return result; } private String computeCommonSuffix() { int end = Math.min(expected.length() - suffixLength + contextLength, expected.length() ); return expected.substring(expected.length() - suffixLength, end) + (expected.length() - suffixLength < 298 Инфраструктура JUnit 299 expected.length() - contextLength ? ELLIPSIS : ""); } Все +1 в computeCommonSuffix были заменены на -1 в charFromEnd , где это смотрится абсолютно логично; также были изменены два оператора <= в suffixOverlapsPre- fix , где это тоже абсолютно логично . Это позволило переименовать suffixIndex в suffixLength , с заметным улучшением удобочитаемости кода . Однако здесь возникла одна проблема . В ходе устранения +1 я заметил в compact- String следующую строку: if (suffixLength > 0) Найдите ее в листинге 15 .4 . Так как suffixLength стало на 1 меньше, чем было пре- жде, мне следовало бы заменить оператор > оператором >= , но это выглядит нело- гично . При более внимательном анализе мы видим, что команда if предотвращает присоединение суффикса с нулевой длиной . Но до внесения изменений команда if была бесполезной, потому что значение suffixIndex не могло быть меньше 1! Это ставит под сомнение полезность обеих команд if в compactString ! Похоже, обе команды можно исключить . Закомментируем их и проведем тестирование . Тесты прошли! Давайте изменим структуру compactString , чтобы удалить лишние команды if и значительно упростить самую функцию [G9] . private String compactString(String source) { return computeCommonPrefix() + DELTA_START + source.substring(prefixLength, source.length() - suffixLength) + DELTA_END + computeCommonSuffix(); } Стало гораздо лучше! Теперь мы видим, что функция compactString просто соеди- няет фрагменты строки . Вероятно, этот факт можно сделать еще более очевид- ным . Осталось еще много мелких улучшений, которые можно было бы внести в код . Но я не стану мучить вас подробными описаниями остальных изменений и просто приведу окончательный результат в листинге 15 .5 . листинг 15 .5 . ComparisonCompactor.java (окончательная версия) package junit.framework; public class ComparisonCompactor { private static final String ELLIPSIS = "..."; private static final String DELTA_END = "]"; private static final String DELTA_START = "["; private int contextLength; private String expected; private String actual; продолжение 299 300 Глава 15 . Внутреннее строение JUnit листинг 15 .5 (продолжение) private int prefixLength; private int suffixLength; public ComparisonCompactor( int contextLength, String expected, String actual ) { this.contextLength = contextLength; this.expected = expected; this.actual = actual; } public String formatCompactedComparison(String message) { String compactExpected = expected; String compactActual = actual; if (shouldBeCompacted()) { findCommonPrefixAndSuffix(); compactExpected = compact(expected); compactActual = compact(actual); } return Assert.format(message, compactExpected, compactActual); } private boolean shouldBeCompacted() { return !shouldNotBeCompacted(); } private boolean shouldNotBeCompacted() { return expected == null || actual == null || expected.equals(actual); } private void findCommonPrefixAndSuffix() { findCommonPrefix(); suffixLength = 0; for (; !suffixOverlapsPrefix(); suffixLength++) { if (charFromEnd(expected, suffixLength) != charFromEnd(actual, suffixLength) ) break; } } private char charFromEnd(String s, int i) { return s.charAt(s.length() - i - 1); } private boolean suffixOverlapsPrefix() { return actual.length() - suffixLength <= prefixLength || expected.length() - suffixLength <= prefixLength; } 300 Инфраструктура JUnit 301 private void findCommonPrefix() { prefixLength = 0; int end = Math.min(expected.length(), actual.length()); for (; prefixLength < end; prefixLength++) if (expected.charAt(prefixLength) != actual.charAt(prefixLength)) break; } private String compact(String s) { return new StringBuilder() .append(startingEllipsis()) .append(startingContext()) .append(DELTA_START) .append(delta(s)) .append(DELTA_END) .append(endingContext()) .append(endingEllipsis()) .toString(); } private String startingEllipsis() { return prefixLength > contextLength ? ELLIPSIS : ""; } private String startingContext() { int contextStart = Math.max(0, prefixLength - contextLength); int contextEnd = prefixLength; return expected.substring(contextStart, contextEnd); } private String delta(String s) { int deltaStart = prefixLength; int deltaEnd = s.length() - suffixLength; return s.substring(deltaStart, deltaEnd); } private String endingContext() { int contextStart = expected.length() - suffixLength; int contextEnd = Math.min(contextStart + contextLength, expected.length()); return expected.substring(contextStart, contextEnd); } private String endingEllipsis() { return (suffixLength > contextLength ? ELLIPSIS : ""); } } Результат выглядит вполне симпатично . Модуль делится на группы: первую группу составляют функции анализа, а вторую — функции синтеза . Функции топологически отсортированы таким образом, что определение каждой функции размещается перед ее первым использованием . Сначала определяются все функ- ции анализа, а за ними следуют функции синтеза . 301 302 Глава 15 . Внутреннее строение JUnit Внимательно присмотревшись, можно заметить, что я отменил некоторые ре- шения, принятые ранее в этой главе . Например, некоторые извлеченные ме- тоды были снова встроены в formatCompactedComparison , а смысл выражения shouldNotBeCompacted снова изменился . Это типичная ситуация . Одна переработка часто приводит к другой, отменяющей первую . Переработка представляет собой итеративный процесс, полный проб и ошибок, но этот процесс неизбежно при- водит к формированию кода, достойного настоящего профессионала . Заключение Итак, «правило бойскаута» выполнено: модуль стал чище, чем был до нашего прихода . И дело не в том, что он был недостаточно чист, — авторы отлично по- трудились над ним . Однако не существует модуля, который нельзя было бы улучшить, и каждый из нас обязан оставить чужой код хотя бы немного лучше, чем он был . 302 Переработка SerialDate Посетив страницу http://www.jfree.org/jcommon/index.php, вы найдете на ней опи- сание библиотеки JCommon . Глубоко в недрах этой библиотеки скрыт пакет org. jfree.date . Пакет содержит класс с именем SerialDate . В этой главе мы займемся анализом этого класса . Класс SerialDate написан Дэвидом Гилбертом (David Gilbert) . Несомненно, Дэвид является опытным и компетентным программистом . Как вы сами убедитесь, в этом коде он проявил значительную степень профессионализма и дисциплины . Во всех отношениях это «хороший код» . А сейчас я намерен разнести его в пух и прах . Дело вовсе не в злом умысле . И я вовсе не считаю, что я намного лучше Дэвида и поэтому имею право критиковать его код . Действительно, если заглянуть в мой код, я уверен, что вы найдете в нем немало поводов для критики . 16 303 304 Глава 16 . Переработка SerialDate Нет, дело не в моем скверном характере или надменности . Я всего лишь намерен проанализировать код с профессиональной точки зрения, не более и не менее . Это то, что все мы должны делать спокойно и без угрызений совести . И все мы должны только приветствовать, когда такой анализ кто-то проводит за нас . Только после подобной критики мы узнаем нечто новое . Это делают врачи . Это делают пилоты . Это делают адвокаты . И мы, программисты, тоже должны этому научиться . И еще одно замечание по поводу Дэвида Гилберта: Дэвид — не просто хороший программист . У него хватило смелости и доброй воли на то, чтобы бесплатно предоставить свой код сообществу . Дэвид разместил свой код в открытом до- ступе и предложил всем желающим использовать и обсуждать его . Отличная работа! Класс SerialDate (листинг Б .1, с . 390) представляет даты в языке Java . Зачем нужен класс для представления дат, если в Java уже имеются готовые классы java.util.Date , java.util.Calendar и т . д .? Автор написал свой класс из-за про- блемы, с которой часто сталкивался сам . Ее суть хорошо разъясняется в от- крывающем комментарии Javadoc (строка 67) . Возможно, кому-то такое решение покажется радикальным, но мне и самому приходилось сталкиваться с этой проблемой, и я приветствую класс, ориентированный на работу с датой вместо времени . |