IT eSky.pl

RSS

Jeśli nie DRY, to co?

Jakiś czas temu przeglądając kod aplikacji natrafiłem na kawałek kodu, który po znacznym uproszczeniu mógłby wyglądać mniej więcej tak:

Kilka słów o tym co tu się dzieje. Klasy służą do mapowania obiektu faktury na obiekt DTO.
Mamy klasę rodzica GenericInvoiceMapper, po której dziedziczą klasy NormalInvoiceMapper i ProformInvoiceMapper mapujące na konkretne obiekty DTO w zależności od potrzeby.

Co z tym kodem jest nie tak?

Na pierwszy rzut oka widać, że instancją klasy GenericInvoiceMapper nie wiele jesteśmy w stanie zrobić – tworzenie jej obiektu nie ma sensu. Powinna być klasą abstrakcyjną. Brakuje też abstrakcyjnej metody map.

DRY

Jednak to co jest tu najbardziej interesujące możemy znaleźć w klasach potomnych: NormalInvoiceMapper i ProformInvoiceMapper. Widać, że klasy są do siebie podobne… a za razem na tyle się różnią, że ciężko wydzielić część wspólną. Algorytm mapowania jest taki sam, natomiast wykonywany jest na różnych obiektach, różniącymi się po między sobą nazwami atrybutów. Dodatkowo metody addDocumentItem korzystają z różnych maperów.

Tak naprawdę mamy tutaj problem z DRY. Nie widać tego od razu bo to nie zwykły „copy-paste”. Programista zwyczajnie tego nie zauważył albo nie wiedział co z tym zrobić.

Więc co z tym zrobić?

Najprościej jest pójść tropem klasy i metod abstrakcyjnych. Wyciągamy różnice w algorytmie do metod i określamy te metody jako abstrakcyjne w klasie GenericInvoiceMapper. Klasa rodzica jako jedyna powinna mieć metodę ::map() zawierającą uwspólniony algorytm i pobierać odpowiednie nazwy atrybutów czy instancje klas z abstrakcyjnych metod nadpisanych w klasach potomnych. Po refactoringu kod mógłby wyglądać w następujący sposób:

To co widać powyżej nie jest niczym innym jak wykorzystaniem wzorca Template Method. Podsumowując: pozbyliśmy się zaśmiecającego kod DRY i wykorzystaliśmy do tego stary jak świat wzorzec projektowy. Wygląda na to, że wszystko jest OK… ale czy na pewno?

Co tu znowu jest nie tak?

Problem pojawia się w momencie gdy chcielibyśmy takie rozwiązanie przetestować jednostkowo. Testowanie klas za pomocą unit testów powinno odbywać się w jak największej izolacji. I nagle okazuje się, że nie jesteśmy w stanie samodzielnie przetestować klasy abstrakcyjnej ani klasy potomnej. Z Template Method jest taki problem, że używa dziedziczenia, a dziedziczenie wprowadza bardzo silną zależność po między klasami. I znowu jest problem…

I co teraz z tym zrobić?

Skoro problemem okazuje się dziedziczenie to trzeba się go pozbyć. I jak się okazuje wcale to nie jest trudne. Idziemy w dobrym kierunku i mamy już częściowo zrefaktoryzowany kod, który łatwo można przerobić na wzorzec strategi.

Klasa GenericInvoiceMapper na na powrót przestaje być abstrakcyjna. Jej abstrakcyjne metody wyciągamy do interfejsu. Klasy mapperów: NormalInvoiceMapper i ProformInvoiceMapper zamieniamy na strategie implementujące interface. One zaś będą wstrzykiwane do głównej klasy GenericInvoiceMapper, znającej algorytm mapujący.

Jak widać powyższy kod diametralnie różni się od tego co zastaliśmy na początku. Nie ma DRY, nie ma Template Method i dziedziczenia, nie ma problemu z testami bo każdą z klas jesteśmy w stanie przetestować osobno.


Zobacz również