Создание, анализ ирефакторинг
 Скачать 3.16 Mb.
|
|
Прежде всего — заставить работать В классе SerialDateTests содержится набор модульных тестов (листинг Б .2, с . 411) . Все тесты проходят . К сожалению, беглое изучение тестов показывает, что тестирование не покрывает часть кода [T1] . Например, поиск показывает, что метод MonthCodeToQuarter (строка 334) не используется [F4] . Соответственно он не включается в модульные тесты . Итак, я запустил Clover, чтобы узнать, какая часть кода реально покрывается модульными тестами . Clover сообщает, что модульные тесты выполняют только 91 из 185 исполняемых команд SerialDate (около 50%) [T2] . Карта покрытия на- поминала лоскутное одеяло, а по всему классы были разбросаны большие пятна невыполняемого кода . Моей целью было полное понимание и переработка кода этого класса . Я не мог добиться этого без значительного улучшения тестового покрытия, поэтому мне пришлось написать собственный набор абсолютно независимых модульных тестов (листинг Б .4, с . 419) . Просматривая код тестов, можно заметить, что многие из них закомментиро- ваны . Эти тесты не проходили в исходном варианте . Однако они представляют поведение, которым, на мой взгляд, должен обладать класс SerialDate . Соответ- ственно, в ходе переработки SerialDate я буду работать над тем, чтобы эти тесты тоже проходили . 304 Прежде всего — заставить работать 305 Даже с несколькими закомментированными тестами Clover сообщает, что новые модульные тесты покрывают 170 (92%) из 185 исполняемых команд . Неплохо, хотя я думаю, что и этот показатель можно улучшить . Возможно, в нескольких первых закомментированных тестах (строки 23–63) я слегка хватил через край . Их прохождение не было формально заложено при проектировании программы, но данное поведение казалось мне абсолютно оче- видным [G2] . Я не знаю, зачем создавался метод testWeekdayCodeToString , но раз уж он был на- писан, казалось очевидным, что в работе метода не должен учитываться регистр символов . Написать соответствующий тест было элементарно [T3] . Заставить его работать было еще проще; я просто изменил строки 259 и 263, чтобы в них использовалась функция equalsIgnoreCase Тесты в строках 32 и 45 остались закомментированными, так как мне было не- ясно, нужно ли поддерживать сокращения вида «tues» и «thurs» . Тесты в строках 153 и 154 не проходят . Хотя, естественно, должны про ходить [G2] . Проблема (а заодно и тесты в строках 163–213) легко исправляется внесе- нием следующих изменений в функцию stringToMonthCode 457 if ((result < 1) || (result > 12)) { result = -1; 458 for (int i = 0; i < monthNames.length; i++) { 459 if (s.equalsIgnoreCase(shortMonthNames[i])) { 460 result = i + 1; 461 break; 462 } 463 if (s.equalsIgnoreCase(monthNames[i])) { 464 result = i + 1; 465 break; 466 } 467 } 468 } Закомментированный тест в строке 318 выявляет ошибку в методе get- FollowingDayOfWeek (строка 672) . 25 декабря 2004 года было субботой . Сле- дующей субботой было 1 января 2005 года . Тем не менее при запуске теста getFollowingDayOfWeek утверждает, что первой субботой, предшествующей 25 де- кабря, было 25 декабря . Разумеется, это неверно [G3],[T1] . Проблема — типичная ошибка граничного условия [T5] — кроется в строке 685 . Строка должна читаться следующим образом: 685 if (baseDOW >= targetWeekday) { Интересно, что проблемы с этой функцией возникали и раньше . Из исто- рии изменений (строка 43) видно, что в функциях getPreviousDayOfWeek , getFollowingDayOfWeek и getNearestDayOfWeek [T6] «исправлялись ошибки» . Модульный тест testGetNearestDayOfWeek (строка 329), проверяющий работу метода getNearestDayOfWeek (строка 705), изначально был не таким длинным 305 306 Глава 16 . Переработка SerialDate и исчерпывающим, как в окончательной версии . Я включил в него много допол- нительных тестовых сценариев, потому что не все исходные тесты проходили успешно [T6] . Посмотрите, какие тестовые сценарии были закомментирова- ны — закономерность проявляется достаточно очевидно [T7] . Сбой в алгоритме происходит в том случае, если ближайший день находится в будущем . Очевидно, и здесь происходит какая-то ошибка граничного условия [T5] . Результаты тестового покрытия кода, полученные от Clover, тоже весьма интерес- ны [T8] . Строка 719 никогда не выполняется! Следовательно, условие if в строке 718 всегда ложно . С первого взгляда на код понятно, что это действительно так . Переменная adjust всегда отрицательна, она не может быть больше либо равна 4 . Значит, алгоритм попросту неверен . Правильный алгоритм выглядит так: int delta = targetDOW - base.getDayOfWeek(); int positiveDelta = delta + 7; int adjust = positiveDelta % 7; if (adjust > 3) adjust -= 7; return SerialDate.addDays(adjust, base); Наконец, для прохождения тестов в строках 417 и 429 достаточно инициировать исключение IllegalArgumentException вместо возвращения строки ошибки в функ- циях weekInMonthToString и relativeToString После таких изменений все модульные тесты проходят . Вероятно, класс Serial- Date теперь действительно работает . Теперь пришло время «довести его до ума» . …Потом очистить код Мы проанализируем код SerialDate от начала до конца, усовершенствуя его в ходе просмотра . Хотя ниже об этом не упоминается, после каждого вносимого изме- нения выполнялись все модульные тесты JCommon, включая мой доработанный модульный тест SerialDate . Итак, вы можете быть уверены в том, что вносимые изменения не нарушают работы JCommon . Начнем со строки 1 . В ней приводятся многословные комментарии с информа- цией о лицензии, авторских правах, авторах и истории изменений . Бесспорно, существуют некоторые юридические формальности, которые необходимо соблю- дать, поэтому авторские права и лицензии должны остаться . С другой стороны, история изменений является пережитком из 1960-х годов . Сегодня у нас имеются системы управления исходным кодом, которые все это делают за нас . Историю следует удалить [C1] . Список импорта, начинающийся в строке 61, следует сократить при помощи конструкций java.text.* и java.util.* [J1] . К форматированию HTML в Javadoc (строка 67) я отношусь без восторга . Меня беспокоят исходные файлы, написанные более чем на одном языке . В этом ком- 306 …Потом очистить код 307 ментарии встречаются четыре языка: Java, английский, Javadoc и HTML [G1] . При таком количестве языков трудно поддерживать порядок в коде . Например, аккуратное размещение строк 71 и 72 теряется при генерировании кода Javadoc, да и кому захочется видеть теги
и 308 Глава 16 . Переработка SerialDate но это неудачная мысль [J2] . MonthConstants следовало бы оформить в виде пере- числения . public abstract class DayDate implements Comparable, Serializable { public static enum Month { JANUARY(1), FEBRUARY(2), MARCH(3), APRIL(4), MAY(5), JUNE(6), JULY(7), AUGUST(8), SEPTEMBER(9), OCTOBER(10), NOVEMBER(11), DECEMBER(12); Month(int index) { this.index = index; } public static Month make(int monthIndex) { for (Month m : Month.values()) { if (m.index == monthIndex) return m; } throw new IllegalArgumentException("Invalid month index " + monthIndex); } public final int index; } Преобразование MonthConstants в enum инициирует ряд изменений в классе DayDate и всех его пользователях . На внесение всех изменений мне потребовалось около часа . Однако теперь любая функция, прежде получавшая int вместо месяца, теперь получает значение из перечисления Month . Это означает, что мы можем удалить метод isValidMonthCode (строка 326), а также все проверки ошибок кодов месяцев — например, monthCodeToQuarter (строка 356) [G5] . Далее возьмем строку 91, serialVersionUID . Переменная используется для управ- ления сериализацией данных . Если изменить ее, то данные DayDate , записанные старой версией программы, перестанут читаться, а попытки приведут к исклю- чению InvalidClassException . Если вы не объявите переменную serialVersionUID , компилятор автоматически сгенерирует ее за вас, причем значение переменной будет различаться при каждом внесении изменений в модуль . Я знаю, что во всей документации рекомендуется управлять этой переменной вручную, но мне кажется, что автоматическое управление сериализацией надежнее [G4] . В кон- це концов, я предпочитаю отлаживать исключение InvalidClassException , чем необъяснимое поведение программы в результате того, что я забыл изменить serialVersionUID . Итак, я собираюсь удалить эту переменную — по крайней мере пока � 308 …Потом очистить код 309 Комментарий в строке 93 выглядит избыточным . Избыточные комментарии толь- ко распространяют лживую и недостоверную информацию [C2] . Соответственно, я удаляю его вместе со всеми аналогами . В комментариях в строках 97 и 100 упоминаются порядковые номера, о которых говорилось ранее [C1] . Комментарии описывают самую раннюю и самую позд- нюю дату, представляемую классом DayDate . Их можно сделать более понятными [N1] . public static final int EARLIEST_DATE_ORDINAL = 2; // 1/1/1900 public static final int LATEST_DATE_ORDINAL = 2958465; // 12/31/9999 Мне неясно, почему значение EARLIEST_DATE_ORDINAL равно 2, а не 0 . Комментарий в строке 829 подсказывает, что это как-то связано с представлением дат в Micro- soft Excel . Более подробное объяснение содержится в производном от DayDate классе с именем SpreadsheetDate (листинг Б .5, с . 428) . Комментарий в строке 71 хорошо объясняет суть дела . Проблема в том, что такой выбор относится к реализации SpreadsheetDate и не имеет ничего общего с DayDate . Из этого я заключаю, что EARLIEST_DATE_ORDINAL и LATEST_DATE_ORDINAL реально не относятся к DayDate и их следует переместить в SpreadsheetDate [G6] . Поиск по коду показывает, что эти переменные используются только в Spread- sheetDate . Они не используются ни в DayDate , ни в других классах JCommon . Со- ответственно, я перемещаю их в SpreadsheetDate Со следующими переменными, MINIMUM_YEAR_SUPPORTED и MAXIMUM_YEAR_SUPPORTED (строки 104 и 107), возникает дилемма . Вроде бы понятно, что если DayDate явля- ется абстрактным классом, то он не должен содержать информации о минималь- ном или максимальном годе . У меня снова возникло искушение переместить эти переменные в SpreadsheetDate [G6] . Тем не менее поиск показал, что эти перемен- ные используются еще в одном классе: RelativeDayOfWeekRule (листинг Б .6, с . 438) . В строках 177 и 178 функция getDate проверяет, что в ее аргументе передается действительный год . Дилемма состоит в том, что пользователю абстрактного класса необходима информация о его реализации . Наша задача — предоставить эту информацию, не загрязняя самого класса DayDate . В общем случае мы могли бы получить данные реализации из экземпля- ра производного класса, однако функция getDate не получает экземпляр DayDate С другой стороны, она возвращает такой экземпляр, а это означает, что она его где-то создает . Из строк 187–205 можно заключить, что экземпляр DayDate созда- ется при вызове одной из трех функций: getPreviousDayOfWeek , getNearestDayOfWeek или getFollowingDayOfWeek . Обратившись к листингу DayDate , мы видим, что все эти функции (строки 638–724) возвращают дату, созданную функцией addDays (строка 571), которая вызывает createInstance (строка 808), которая создает SpreadsheetDate ! [G7] . В общем случае базовые классы не должны располагать информацией о своих производных классах . Проблема решается применением паттерна АБСТРАКТ- 309 310 Глава 16 . Переработка SerialDate НАЯ ФАБРИКА [GOF] и созданием класса DayDateFactory . Фабрика создает экземпляры DayDate , а также предоставляет информацию по поводу реализации — в частности, минимальное и максимальное значение даты . public abstract class DayDateFactory { private static DayDateFactory factory = new SpreadsheetDateFactory(); public static void setInstance(DayDateFactory factory) { DayDateFactory.factory = factory; } protected abstract DayDate _makeDate(int ordinal); protected abstract DayDate _makeDate(int day, DayDate.Month month, int year); protected abstract DayDate _makeDate(int day, int month, int year); protected abstract DayDate _makeDate(java.util.Date date); protected abstract int _getMinimumYear(); protected abstract int _getMaximumYear(); public static DayDate makeDate(int ordinal) { return factory._makeDate(ordinal); } public static DayDate makeDate(int day, DayDate.Month month, int year) { return factory._makeDate(day, month, year); } public static DayDate makeDate(int day, int month, int year) { return factory._makeDate(day, month, year); } public static DayDate makeDate(java.util.Date date) { return factory._makeDate(date); } public static int getMinimumYear() { return factory._getMinimumYear(); } public static int getMaximumYear() { return factory._getMaximumYear(); } } Фабрика заменяет методы createInstance методами makeDate , в результа- те чего имена выглядят гораздо лучше [N1] . По умолчанию используется SpreadsheetDateFactory , но этот класс можно в любой момент заменить другой фа- брикой . Статические методы, делегирующие выполнение операций абстрактным методам, используют комбинацию паттернов СИНГЛЕТ [GOF], ДЕКОРАТОР [GOF] и АБСТРАКТНАЯ ФАБРИКА . Класс SpreadsheetDateFactory выглядит так: public class SpreadsheetDateFactory extends DayDateFactory { public DayDate _makeDate(int ordinal) { return new SpreadsheetDate(ordinal); } public DayDate _makeDate(int day, DayDate.Month month, int year) { return new SpreadsheetDate(day, month, year); } public DayDate _makeDate(int day, int month, int year) { return new SpreadsheetDate(day, month, year); } 310 …Потом очистить код 311 public DayDate _makeDate(Date date) { final GregorianCalendar calendar = new GregorianCalendar(); calendar.setTime(date); return new SpreadsheetDate( calendar.get(Calendar.DATE), DayDate.Month.make(calendar.get(Calendar.MONTH) + 1), calendar.get(Calendar.YEAR)); } protected int _getMinimumYear() { return SpreadsheetDate.MINIMUM_YEAR_SUPPORTED; } protected int _getMaximumYear() { return SpreadsheetDate.MAXIMUM_YEAR_SUPPORTED; } } Как видите, я уже переместил переменные MINIMUM_YEAR_SUPPORTED и MAXIMUM_YEAR_ SUPPORTED в класс SpreadsheetDate , в котором им положено находиться [G6] . Следующая проблема DayDate — константы дней, начинающиеся со строки 109 . Их следует оформить в виде другого перечисления [J3] . Мы уже видели, как это делается, поэтому я не буду повторяться . При желании посмотрите в итоговом листинге . Далее мы видим серию таблиц, начинающуюся с LAST_DAY_OF_MONTH в строке 140 . Моя первая претензия к этим таблицам состоит в том, что описывающие их ком- ментарии избыточны [C3] . Одних имен вполне достаточно, поэтому я собираюсь удалить комментарии . Также неясно, почему эта таблица не объявлена приватной [G8], потому что в классе имеется статическая функция lastDayOfMonth , предоставляющая те же данные . Следующая таблица, AGGREGATE_DAYS_TO_END_OF_MONTH , выглядит загадочно — она ни разу не используется в JCommon [G9] . Я удалил ее . То же произошло с LEAP_YEAR_AGGREGATE_DAYS_TO_END_OF_MONTH Следующая таблица, AGGREGATE_DAYS_TO_END_OF_PRECEDING_MONTH , используется только в SpreadsheetDate (строки 434 и 473) . Так почему бы не переместить ее в SpreadsheetDate ? Против перемещения говорит тот факт, что таблица не при- вязана ни к какой конкретной реализации [G6] . С другой стороны, никаких реализаций, кроме SpreadsheetDate , фактически не существует, поэтому таблицу следует переместить ближе к месту ее использования [G10] . Для меня решающим обстоятельством является то, что для обеспечения логи- ческой согласованности [G11] таблицу следует объявить приватной и предо- ставить доступ к ней через функцию вида julianDateOfLastDayOfMonth . Но по- хоже, такая функция никому не нужна . Более того, если этого потребует новая реализация DayDate , таблицу можно будет легко вернуть на место . Поэтому я ее переместил . Далее следуют три группы констант, которые можно преобразовать в перечис- ления (строки 162–205) . 311 |