Недавно я пытался улучшить свои способности к рефакторингу с помощью Магазина видео.
Я пробовал только одну другую ката рефакторинга, Позолоченную розу, но я думаю, что обе ката оставили меня с одинаковым чувством замешательства, когда я впервые просмотрел их. Этот пост в блоге подробно описывает начальные шаги, которые я предпринял для рефакторинга кода:
Первые несколько вещей, которые я сделал, были быстрые победы, такие как правильное выравнивание и отступ кода, чтобы я мог лучше понять его.
Тесты в тестовом классе были расположены очень запутанно.
В основном утверждение было объединено в одну большую строку:
"Rental Record for Fred\n\tThe Cell\t9.0\nYou owed 9.0\nYou earned 2 frequent renter points\n"
Это означало не только то, что вам нужно было прокручивать вправо, чтобы прочитать всю строку, но и то, что в коде было сложно выделить самое важное. Чтобы сделать общий замысел тестов более ясным, я разбил эту строку по каждому разрыву строки.
"Rental Record for Fred\n" + "\tThe Cell\t9.0\n" + "You owed 9.0\n" + "You earned 2 frequent renter points\n"
Основная цель заявления заключалась в том, чтобы проинформировать клиента о том, что он должен и каковы его баллы арендатора. Я извлек эти два значения и удалил остальную часть шаблона строки оператора. После того, как я применил этот рефакторинг к остальным тестам, их стало намного легче переваривать, а также я больше сосредоточился на рефакторинге остального кода.
Всю строку по-прежнему нужно было проверить хотя бы один раз, поэтому я оставил один тест, который проверял строку целиком.
public String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = this.rentals.elements(); String result = "Rental Record for " + getName() + "\n"; while (rentals.hasMoreElements()) { double thisAmount = 0; Rental each = (Rental) rentals.nextElement(); switch (each.getMovie().getPriceCode()) { case Movie.REGULAR: thisAmount += 2; if (each.getDaysRented() > 2) thisAmount += (each.getDaysRented() - 2) * 1.5; break case Movie.NEW_RELEASE thisAmount += each.getDaysRented() * 3 break; case Movie.CHILDRENS: thisAmount += 1.5; if (each.getDaysRented() > 3) thisAmount += (each.getDaysRented() - 3) * 1.5; break; } frequentRenterPoints++; if (each.getMovie().getPriceCode() == Movie.NEW_RELEASE && each.getDaysRented() > 1) frequentRenterPoints++; result += "\t" + each.getMovie().getTitle() + "\t" + String.valueOf(thisAmount) + "\n"; totalAmount += thisAmount; } result += "You owed " + String.valueOf(totalAmount) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points\n"; return result; }
В классе Customer значения, напечатанные в последней строке, из которой состоит фактический оператор, были локальными переменными в методе statement(). Там переменные были названы getFrequentRenterPoints и thisAmount. Я преобразовал эти две переменные в переменные экземпляра и создал методы getFrequentRenterPoints() и getAmountOwed(), которые возвращали эти переменные экземпляра, что означало, что я мог проверить правильность этих значений.
Последние несколько рефакторингов, которые я сделал в тестах, заключались в удалении импорта JUnit. Это означало, что я мог удалить строку «extends TestCase», основанную на старом соглашении по тестированию.
Затем я удалил повторяющиеся строки для каждого названия и типа фильма, извлекая строки в поля.
Я назвал извлеченные поля переменными в соответствии с их типами фильмов, поэтому тип нового выпуска стал newRelease1, а затем любые другие фильмы с другими именами, но того же типа фильма, были названы newRelease2 и так далее. .
Я также встраиваю переменные FreightRenterPoints и amountOwed, чтобы сделать тест максимально лаконичным.
public class VideoStoreTest { private final Movie newRelease1 = new Movie("The Cell", Movie.NEW_RELEASE); private Customer customer; BEFORE @Test public void testSingleNewReleaseStatement() { customer.addRental(new Rental(new Movie("The Cell", Movie.NEW_RELEASE), 3)); double amountOwed = 9.0; int frequentRenterPoints = 2; customer.statement(); assertEquals(amountOwed, customer.getAmountOwed()); assertEquals(frequentRenterPoints, customer.getFrequentRenterPoints()); } AFTER @Test public void testSingleNewReleaseStatement() { customer.addRental(new Rental(newRelease1, 3)); customer.statement(); assertEquals(9.0, customer.getAmountOwed()); assertEquals(2, customer.getFrequentRenterPoints()); }
Затем я посмотрел на огромный метод statement() в классе Customer и попытался придумать, как его сократить.
Я знал, что есть два ключевых значения, на которых я хотел сосредоточиться — frequentRenterPoints и totalAmount — и оба они были сильно запутаны в statement() код метода. Глядя на многочисленные строки кода и различные элементы, участвующие в создании общей инструкции, становится ясно, что этот метод делает слишком много.
Я решил использовать метод рефакторинга extract, чтобы изолировать некоторые из этих ключевых элементов.
Первым шагом было выделение логики для получения баллов арендатора в собственный метод:
private void getFrequentRenterPoints(Rental eachMovie) { frequentRenterPoints++; if (eachMovie.getMovie().getPriceCode() == Movie.NEW_RELEASE && eachMovie.getDaysRented() > 1) frequentRenterPoints++; }
Это сократило мой метод statement(), так что он казался полезным рефакторингом. Однако увеличение переменной экземпляра, которую я сделал ранее, означало, что я создавал изменяемое состояние. Мой код стал хрупким.
Чтобы удалить это изменяемое состояние, я посмотрел, что на самом деле делает этот метод:
- Он увеличивал frequentRenterPoints на единицу для каждого фильма.
- Если фильм был новым прокатом ибылпрокатом более одного дня, то к общему количеству баллов добавлялся еще один балл арендатора.
Этот метод был излишне запутанным. Чтобы упростить задачу, я преобразовал переменную экземпляра frequentRenterPoints в локальную переменную с именем points, а затем добавил к баллам 1 или 2 в зависимости от того, соответствует ли фильм условию.
private int frequentRenterPoints(Rental rental) { int points = 0; if (rental.getMovie().getPriceCode() == Movie.NEW_RELEASE && rental.getDaysRented() > 1) { points += 2; } else { points += 1; } return points; }
Затем я понял, что на самом деле мне не нужна переменная points, поскольку я просто добавлял 1 или 2 к 0. Поэтому я удалил ее и сделал метод еще проще.
private int frequentRenterPoints(Rental rental) { if (rental.getMovie().getPriceCode() == Movie.NEW_RELEASE && rental.getDaysRented() > 1) { return 2; } else { return 1; } }
Затем я вернул точки и добавил их в переменную экземпляра frequentRenterPoints в методе основного оператора. Я удалил состояние из этого метода, но идея заключалась в том, чтобы полностью удалить его из моей программы.
В моем следующем сообщении в блоге я расскажу:
- Как я реорганизовал переменную totalAmount.
- Как удалить все изменяемое состояние из моей программы.
- Как я еще больше упростил метод statement(), извлекая оператор case в разные классы и используя полиморфизм.