Недавно я пытался улучшить свои способности к рефакторингу с помощью Магазина видео.

Я пробовал только одну другую ката рефакторинга, Позолоченную розу, но я думаю, что обе ката оставили меня с одинаковым чувством замешательства, когда я впервые просмотрел их. Этот пост в блоге подробно описывает начальные шаги, которые я предпринял для рефакторинга кода:

Первые несколько вещей, которые я сделал, были быстрые победы, такие как правильное выравнивание и отступ кода, чтобы я мог лучше понять его.

Тесты в тестовом классе были расположены очень запутанно.

В основном утверждение было объединено в одну большую строку:

"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(), так что он казался полезным рефакторингом. Однако увеличение переменной экземпляра, которую я сделал ранее, означало, что я создавал изменяемое состояние. Мой код стал хрупким.

Чтобы удалить это изменяемое состояние, я посмотрел, что на самом деле делает этот метод:

  1. Он увеличивал frequentRenterPoints на единицу для каждого фильма.
  2. Если фильм был новым прокатом ибылпрокатом более одного дня, то к общему количеству баллов добавлялся еще один балл арендатора.

Этот метод был излишне запутанным. Чтобы упростить задачу, я преобразовал переменную экземпляра 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 в разные классы и используя полиморфизм.