W definicji Martina Fowlera dotyczącej refaktoryzacji mamy bardzo ważny fragment dotyczący obserwowalnego zachowania. Pod żadnym pozorem nie możemy go zmienić!

Refactoring - a change made to the internal structure of software to make it easier to understand and cheapier to modify without changing its observable behaviour. Martin Fowler

Dokonując zmian w wewnętrznej strukturze oprogramowania podczas refaktoringu obserwowalne zachowanie musi pozostać bez zmian. Jest to naprawdę istotne, żeby nie mieszać ze sobą różnych operacji, których dokonujemy. Jeśli refaktoryzujemy to zakładamy czapkę osoby refaktoryzującej i tyle. Nie naprawiamy błędów, nie dodajemy funkcjonalności. Natomiast po dokonanych zmianach możemy zdjąć naszą aktualną czapkę i założyć nową np. naprawiacza błędów. Jak to wygląda ma w praktyce? Już pokazuję.

1
2
3
4
5
6
7
8
9
10
11
12
13
14

class VehicleValidator {

  void validate(Policy policy) {
    boolean isAnyTooOldVehicle = policy.getVehicles()
        .stream()
        .map(Vehicle::getProductionYear)
        .allMatch(productionYear -> productionYear.isBefore(Year.of(2000)));
    if (isAnyTooOldVehicle) {
      throw new IllegalStateException("we do not handle vehicles older than made in 2000");
    }
    // ... other validators
  }

}

W tym kodzie znajduje się błąd. Okazuje się, że jeśli chociaż jeden z pojazdów został wyprodukowany przed rokiem 2000 to walidacja nie powinna puścić dalej procesu biznesowego. Tak się jednak nie dzieje… Ten stan rzeczy pewnie wyglądałby inaczej, gdyby dany fragment kodu miał testy. Niestety ich nie ma…

W sumie to może nawet nie dziwić. Wystarczy zajrzeć w kod klasy Policy oraz Vehicle. Mają one ogromną ilość pól, które, na potrzeby testów, trzeba by było żmudnie ustawiać jedno za drugim. Dodatkowo te pola mogą mieć zależności między sobą co tym bardziej nie ułatwia sprawy, aby taki test sporządzić. A my tylko na potrzeby tej walidacji potrzebujemy lat produkcji pojazdów. Co więcej zrobić? Skorzystajmy z refaktoryzacji, zobaczymy gdzie ona nas zaprowadzi.

Pierwszą rzeczą jaką warto zrobić jest wyciągnięcie pojazdów obecnych w klasie Policy jako parametr metody. W ten sposób pozbędziemy się potrzeby stworzenia obiektu tej klasy w testach. Tutaj warto mieć na uwadze dwie rzeczy:

  1. Zamiast ekstrakcji parametru lepiej jest na początku przeciążyć metodę zmieniając parametr Policy na kolekcję obiektów Vehicle
  2. Jeśli nowa metoda potrzebowałaby czegoś z klasy Policy to można to dopisać jako kolejne, łatwe do utworzenia parametry (w tym przypadku nie ma takiej konieczności)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17

class VehicleValidator {

  void validate(Set<Vehicle> vehicles) {
    boolean isAnyTooOldVehicle = vehicles.stream()
        .map(Vehicle::getProductionYear)
        .allMatch(productionYear -> productionYear.isBefore(Year.of(2000)));
    if (isAnyTooOldVehicle) {
      throw new IllegalStateException("we do not handle vehicles older than made in 2000");
    }
    // ... other validators
  }

  void validate(Policy policy) {
    validate(policy.getVehicles());
  }

}

No to mamy to. Klienty klasy VehicleValidator nic nie dowiedziały się o tej zmianie. Dalej przepychają obiekt klasy Policy do metody validate. My natomiast wykonaliśmy krok do przystosowania API pod testy. Kolejnym krokiem będzie zamiana zbioru pojazdów na zbiór lat ich produkcji. Możemy to zrobić poprzez wyciągnięcie zmiennej i ekstrakcję metody.

1
2
3
4
5
6
7
8
9
10
11

void validate(Set<Vehicle> vehicles) {
  boolean isAnyTooOldVehicle = vehicles.stream()
      .map(Vehicle::getProductionYear)
      .collect(Collectors.toUnmodifiableSet())
      .stream()
      .allMatch(productionYear -> productionYear.isBefore(Year.of(2000)));
  if (isAnyTooOldVehicle) {
    throw new IllegalStateException("we do not handle vehicles older than made in 2000");
  }
  // ... other validators
}

1
2
3
4
5
6
7
8
9
10
11
12

void validate(Set<Vehicle> vehicles) {
  Set<Year> uniqueProductionYears = vehicles.stream()
      .map(Vehicle::getProductionYear)
      .collect(Collectors.toUnmodifiableSet());
  boolean isAnyTooOldVehicle = uniqueProductionYears
      .stream()
      .allMatch(productionYear -> productionYear.isBefore(Year.of(2000)));
  if (isAnyTooOldVehicle) {
    throw new IllegalStateException("we do not handle vehicles older than made in 2000");
  }
  // ... other validators
}

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

void validate(Set<Vehicle> vehicles) {
  Set<Year> uniqueProductionYears = vehicles.stream()
      .map(Vehicle::getProductionYear)
      .collect(Collectors.toUnmodifiableSet());
  validateIfVehiclesAreNotTooOld(uniqueProductionYears);
  // ... other validators
}

private static void validateIfVehiclesAreNotTooOld(Set<Year> uniqueProductionYears) {
  boolean isAnyTooOldVehicle = uniqueProductionYears.stream()
      .allMatch(productionYear -> productionYear.isBefore(Year.of(2000)));
  if (isAnyTooOldVehicle) {
    throw new IllegalStateException("we do not handle vehicles older than made in 2000");
  }
}

Metoda validateIfVehiclesAreNotTooOld ma interfejs, który nas interesuje. Ale jest to wewnętrzna implementacja klasy VehicleValidator. Przecież nie zwiększymy modyfikatora dostępu tylko na potrzeby testów, prawda? Nooo, nie do końca. Osoby o słabych nerwach prosi się o zamknięcie oczu.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

void validate(Set<Vehicle> vehicles) {
  Set<Year> uniqueProductionYears = vehicles.stream()
      .map(Vehicle::getProductionYear)
      .collect(Collectors.toUnmodifiableSet());
  validateIfVehiclesAreNotTooOld(uniqueProductionYears);
  // ... other validators
}

static void validateIfVehiclesAreNotTooOld(Set<Year> uniqueProductionYears) {
  boolean isAnyTooOldVehicle = uniqueProductionYears.stream()
      .allMatch(productionYear -> productionYear.isBefore(Year.of(2000)));
  if (isAnyTooOldVehicle) {
    throw new IllegalStateException("we do not handle vehicles older than made in 2000");
  }
}

Teraz napisanie testu będzie trywialne. Ale zanim przejdziemy do tego, mały komentarz. Należy pamiętać jaki cel w tym momencie chcemy osiągnąć. Oczywiście chcemy naprawić błąd. Jednak będąc odpowiedzialnymi programistami musimy upewnić się, że taki błąd nie wystąpi ponownie. A jak najlepiej to zrobić? Pisząc test, który będzie naszą siatką bezpieczeństwa. Aby tego dokonać musieliśmy zmienić tak kod, aby napisanie testu było łatwe i przyjemne, ale przede wszystkim możliwe i utrzymywalne. Być może kroki, które musieliśmy wykonać mówią nam o złym design naszego kodu. Ale na ten moment to nie jest istotne. Warto mieć to jednak na uwadze.

1
2
3
4
5

@Test
void any_vehicle_cannot_be_too_old() {
  assertThrows(IllegalStateException.class, () -> VehicleValidator.validateIfVehiclesAreNotTooOld(
      Set.of(Year.of(1999), Year.of(2001))));
}

Powyższy test nie przechodzi tak jak się tego spodziewaliśmy. Istotne jest w tym momencie to co przekazałem wcześniej. Kończymy z procesem refaktoryzacji. Naprawianie błędów to zmiana obserwowalnego zachowania. Możemy teraz zrobić commita i zabrać się za naprawdę błędu. Okazuje się, że w kodzie produkcyjnym ktoś zamiast anyMatch na Stream, skorzystał z allMatch. Po tej poprawce test nam przejdzie. My poprawiliśmy błąd i mamy poczucie bezpieczeństwa, że on nie wystąpi w przyszłości.

Wracając na chwilę do usunięcia modyfikatora private. Dla tych, którzy nie mogą się z tym pogodzić też coś mam. Możemy sobie utworzyć znacznik w postaci adnotacji, który będzie nam mówiła, że ta metoda może być używana tylko na potrzeby testów.

1
2
3
4

@Target(ElementType.METHOD)
@Retention(RetentionPolicy.SOURCE)
@interface OnlyTestUsage {
}

1
2
3
4
5
6
7
8
9

@OnlyTestUsage
static void validateIfVehiclesAreNotTooOld(final Set<Year> uniqueProductionYears) {
  boolean isAnyTooOldVehicle = uniqueProductionYears
      .stream()
      .allMatch(productionYear -> productionYear.isBefore(Year.of(2000)));
  if (isAnyTooOldVehicle) {
    throw new IllegalStateException("we do not handle vehicles older than made in 2000");
  }
}

Można się umówić, że z takich metod nie korzystamy w kodzie produkcyjnym. Natomiast jeśli dla kogoś to za mało, to warto napisać sobie regułę np. w ArchUnit, która będzie nam wyłapywała takie użycia. W przypadku ich wystąpienia po prostu nie będzie można zbudować projektu. Powstaje pytanie - czy to jest konieczne? To pozostawiam Tobie pod dyskusję.

Na koniec chciałbym jeszcze raz przypomnieć. Naprawianie błędów to zmiana obserwowalnego zachowania. Dlatego pod żadnym pozorem nie powinno się tego robić podczas refaktoryzacji. Warto to sobie odnotować.