Jedna z najprostszych refaktoryzacji wprowadzających wzorzec projektowy do kodu. Wzorcem tym jest metoda szablonowa (template method), a refaktoryzacją wydzielenie metody (extract method). Czyli coś dla n00b-ów 😉

Od czego zaczynamy

Poniżej przykładowy kod, który w tym przypadku sprawdza czy dane wyrażenie przyjmie obiekt danego typu

Listing 1. Sytuacja wyjściowa

public final class SearchHelper {

    private static List<Class> numberClasses = Lists.newArrayList(Long.class, Double.class, Float.class, BigDecimal.class, Integer.class);

    private static List<Class> timeClasses = Lists.newArrayList(Timestamp.class);

    public static boolean isStringEntityField(Expression ep) {
        return !(isNumberEntityField(ep) || isTimeEntityField(ep));
    }

    public static boolean isNumberEntityField(Expression ep) {
        return numberClasses.stream()
                .filter(c -> ep.getJavaType().isAssignableFrom(c))
                .findFirst().isPresent();
    }

    public static boolean isTimeEntityField(Expression ep) {
        return timeClasses.stream()
                .filter(c -> ep.getJavaType().isAssignableFrom(c))
                .findFirst().isPresent()
    }
    /// reszta kodu, nie istotne
}

to co chcemy osiągnąć to jak największe uproszczenie tego kodu tak by operować na dwóch niezależnych bytach – danych oraz pewnej generalniej logice.

Pierwsze zmiany

Mamy w tym kodzie kilka niezbyt ciekawych konstrukcji. Pierwszym co rzuca się w oczy to sposób przeszukiwania kolekcji. W ogólności jest to dobre podejście (zamiana pętli na deklaratywne filtrowanie), ale nie tak 🙂

Listing 2. Upraszczamy przeszukiwanie

public final class SearchHelper {

    private static List<Class> numberClasses = Lists.newArrayList(Long.class, Double.class, Float.class, BigDecimal.class, Integer.class);

    private static List<Class> timeClasses = Lists.newArrayList(Timestamp.class);

    public static boolean isStringEntityField(Expression ep) {
        return !(isNumberEntityField(ep) || isTimeEntityField(ep));
    }

    public static boolean isNumberEntityField(Expression ep) {
        return numberClasses.stream()
                .anyMatch(c -> ep.getJavaType().isAssignableFrom(c));
    }

    public static boolean isTimeEntityField(Expression ep) {
        return timeClasses.stream()
                .anyMatch(c -> ep.getJavaType().isAssignableFrom(c));
    }
    /// reszta kodu, nie istotne
}

Ta zmiana pozwala nam na uproszczenie kodu oraz, co ciekawsze, na lepszą pracę narzędzi do wykrywania duplikatów. Zatem „za darmo” otrzymaliśmy tu pomoc IDE. Kolejnym krokiem jest właściwa refaktoryzacja.

Wydzielanie mięska

Skoro mamy już trochę ogarnięty kod to bardzo łatwo wydzielimy z niego część wspólną dla metod isNumberEntityField i isTimeEntityField.

Listing 3. Wydzielenie metody

public final class SearchHelper {

    private static List<Class> numberClasses = Lists.newArrayList(Long.class, Double.class, Float.class, BigDecimal.class, Integer.class);

    private static List<Class> timeClasses = Lists.newArrayList(Timestamp.class);

    public static boolean isStringEntityField(Expression ep) {
        return !(isNumberEntityField(ep) || isTimeEntityField(ep));
    }

    public static boolean isNumberEntityField(Expression ep) {
        return isIn(numberClasses, ep);
    }

    public static boolean isTimeEntityField(Expression ep) {
        return isIn(timeClasses, ep);
    }

    private static boolean isIn(List<Class> cs,  Expression ep) {
        return cs.stream()
                .anyMatch(c -> ep.getJavaType().isAssignableFrom(c));
    }
    /// reszta kodu, nie istotne
}

I już jest ładnie. Mamy wydzieloną logikę w postaci metody isIn i odseparowaną od danych – list i testowanego wyrażenia. Metody isNumberEntityField i isTimeEntityField są tylko nazwanymi przypadkami przetwarzania.

Deserek

Skoro jest ładnie to może być też bardzo ładnie. Jeżeli spojrzymy na metodę isStringEntityField to zauważymy, że i ona powinna realizować nasz schemat. Co prawda działa ona w oparciu o dwie pozostałe metody (jeżeli nie numer i nie Timestamp to String), ale jest troszkę zagmatwana. Jakiś operator logiczny, jakieś przeczenie… no, ok jak sobie ktoś lubi zabawę z kodem imperatywnym to zapewne będzie OK, ale ja chciałbym by wszystkie te metody działały na jedno kopyto. Potrzebujemy zatem listy podobnej do tych co już mamy… no nie do końca, bo zwróćcie uwagę, że Stringiem jest to co nie jest Timestampem i numerem. Zatem np. kontrolka swingowa powinna być Stringiem.
Stworzenie listy wszystkich klas nie będących Timestampem i numerem jest mało wydajnym rozwiązaniem. Najpierw musimy przeanalizować cały classpath, a potem dzielnie iterować po takiej ogromnej kolekcji. Formalnie będzie OK, ale praktycznie już nie.

Ok, zatem powinniśmy napisać weryfikację, która powie, że dane wyrażenie nie należy do żadnej z list:

Listing 4. Wyrażenie dla String

public final class SearchHelper {

    private static List<Class> numberClasses = Lists.newArrayList(Long.class, Double.class, Float.class, BigDecimal.class, Integer.class);

    private static List<Class> timeClasses = Lists.newArrayList(Timestamp.class);

    private static List<Class> notStringClasses = Stream.concat(numberClasses.stream, timeClasses.stream()).collect(Collectors.toList());

    public static boolean isStringEntityField(Expression ep) {
        return !isIn(notStringClasses, ep);
    }

    public static boolean isNumberEntityField(Expression ep) {
        return isIn(numberClasses, ep);
    }

    public static boolean isTimeEntityField(Expression ep) {
        return isIn(timeClasses, ep);
    }

    private static boolean isIn(List<Class> cs,  Expression ep) {
        return cs.stream()
                .anyMatch(c -> ep.getJavaType().isAssignableFrom(c));
    }
    /// reszta kodu, nie istotne
}

Jest lepiej. Udało nam się zredukować ilość operacji do jednego przeczenia. Jest to jedna z opcji implementacji. Inną, która całkowicie wyrzuca operacje logiczne z naszego kodu macie poniżej:

Listing 5. Wyrażenie dla String bez operatorów logicznych

public final class SearchHelper {

    private static List<Class> numberClasses = Lists.newArrayList(Long.class, Double.class, Float.class, BigDecimal.class, Integer.class);

    private static List<Class> timeClasses = Lists.newArrayList(Timestamp.class);

    private static List<Class> notStringClasses = Stream.concat(numberClasses.stream, timeClasses.stream()).collect(Collectors.toList());

    public static boolean isStringEntityField(Expression ep) {
        return isNotIn(notStringClasses, ep);
    }

    public static boolean isNumberEntityField(Expression ep) {
        return isIn(numberClasses, ep);
    }

    public static boolean isTimeEntityField(Expression ep) {
        return isIn(timeClasses, ep);
    }

    private static boolean isIn(List<Class> cs,  Expression ep) {
        return cs.stream()
                .anyMatch(c -> ep.getJavaType().isAssignableFrom(c));
    }

    private static boolean isNotIn(List<Class> cs,  Expression ep) {
        return cs.stream()
                .noneMatch(c -> ep.getJavaType().isAssignableFrom(c));
    }
    /// reszta kodu, nie istotne
}

Ta wersja kodu jest już ostateczna. Wydzielenie dodatkowej metody nie zawsze jest potrzebne. W naszym kodzie mamy więcej metod negujących isIn zatem ta zmiana jest bardzo naturalna.

Podsumowanie

To czego tu nie poruszyłem to problem testowania. Milcząco założyłem, że mamy testy do tego kodu i zmiany są wykonywane w bezpieczny sposób. Jednakże sama procedura refaktoryzacji jest tu na tyle prosta i pozbawiona pułapek, że pisanie dodatkowo o testach tylko sztucznie wydłużyłby kod.

Dla osób początkujących jeszcze krótka lista poleceń, które będą przydatne. Zakładając, że pracujecie w „gołej” IntelliJ Idea:

  • ctrl + shift + a → Find and Replace Code Duplicates – wyszukuje duplikaty kodu i wyświetla je w osobnym oknie.
  • ctrl + alt + m – extract method
  • ctrl + shift + ↑ – przesuwa blok kodu w górę
  • ctrl + shift + ↓ – przesuwa blok kodu w dół

Te skróty ułatwiają życie.