Zróbmy coś bezużytecznego

Wszystko ma sens wtedy, gdy ma sens. Nie tworzysz kodu, którego nie potrzebujesz z kilku powodów. Po pierwsze, może się okazać, że go (o ironio!) nie będziesz potrzebował. Po drugie, dodajesz pracy sobie i innym, ponieważ ktoś musi o ten kod dbać, ktoś będzie go zapewne niejednokrotnie czytał, i tak dalej. Po trzecie, kod który jest niepotrzebny stwarza problemy ze zrozumieniem aplikacji, bo przecież „skoro ktoś to dodał, to jest potrzebne”.

Nie zawsze jednak jest tak, że kod niepotrzebny nie jest nigdzie wykorzystywany. Zdarzają się sytuacje, gdy powstają metody, które co prawda są użyte, ale są jedynie elementem pewnej funkcjonalnej całości i same nie niosą nic poza potencjalnymi problemami.
I o takich tworach chciałbym dzisiaj opowiedzieć.

single responsibility principle, ale to nie wszystko

SRP mówi nam o tym, że każda klasa powinna być odpowiedzialna za tylko jedną rzecz. Ważne jest jednak, aby tworząc własne klasy/metody pamiętać również o tym, że nasze elementy muszą jej przestrzegać przede wszystkim w kontekście całej aplikacji.

O co dokładnie chodzi? Nie tworzysz metody, która zwraca Ci listę wszystkich faktur dla klienta, jeżeli potrzebujesz jedynie te, które nie zostały zapłacone w terminie. Powinieneś zamiast niej stworzyć metodę, która bezpośrednio zwraca listę tych, którymi jesteś zainteresowany. Problematyczne jest to, że patrząc na samą metodę, wydaje się ona poprawna, jednak w kontekście całości zostaje obnażony fakt, że jest niczym więcej tylko elementem pełnej funkcjonalności.

a z polskiego na nasze?

Przejdźmy teraz do przykłady, który zobrazuje to, o czym napisałem wyżej.

Załóżmy, że mamy aplikację, która działa w oparciu o zdarzenia. Każde z nich ma określony stan:

enum EventState {
 CREATED, INITIALIZED, READY_TO_GO, FINALIZED, CANCELED
}

Z różnych względów jedne ze zdarzeń wykonują się dłużej, a inne krócej. Niektórym zdarza się „zawisnąć” w czasie. Dlatego też raz na dzień wysyłamy do odpowiednich osób powiadomienie o wszystkich zdarzeniach, które nie zostały zakończone lub anulowane:

EventsBatch events = EventRepository.getAll().notFinalized().notCanceled();

if (events.notEmpty()) {
 notyify(events);
}

Niby całkiem dobry kod, a jednak nie do końca. Jakie problemy mogą z niego wyniknąć?

Po pierwsze, pogwałcenie SRP. Kod metody nie jest już odpowiedzialny za wysyłanie notyfikacji o określonych zdarzeniach. On również definiuje to, o jakich zdarzeniach powinien powiadamiać.

Po drugie, wyobraźcie sobie, że w pewnym momencie dodajemy nową funkcjonalność i potrzebujemy uzyskać zbiór dokładnie tych samych zdarzeń. Większość developerów zapewne swoje pierwsze kroki skieruje do klasy EventRepository, ale tam dowie się, że niezbędna metoda nie istnieje. Czy będą przeszukiwali cały kod, aby dowiedzieć się, czy ktoś może wcześniej potrzebował czegoś takiego i dana funkcjonalność znajduje się w innym miejscu? Czy ty byś tak zrobił? Czy takie kroki powinny w ogóle być brane pod uwagę? Poza tym, nawet jeżeli ktoś by się zdecydował na przeszukanie całej aplikacji, to nadal są szanse, że nie udałoby się znaleźć tego kodu.

W takiej sytuacji programista stworzyłby taką metodę, co generuje kolejne problemy bezpośrednio wynikające z pogwałcenia DRY. Mamy dwa miejsca, w których realizujemy to samo, więc przy każdej zmianie powinniśmy je modyfikować (np. dochodzi nam stan DUPLICATED, o którym również nie chcemy powiadamiać). Chyba nie trudno sobie wyobrazić, że bardzo łatwo o którymś można zapomnieć.

Poza tym, detale implementacji tego fragmentu kodu wyciekają na zewnątrz, tu już nie ma mowy o enkapsulacji, a co gorsza, to zewnętrzny obiekt dba o poprawny stan obiektu tworzonego (tak naprawdę pula zdarzeń dopóki nie zostanie przefiltrowana nie jest użyteczna, znajduje się w niepoprawnym stanie).

Mamy dwa razy większe szanse na popełnienie błędów oraz musimy po raz kolejny przetestować taką samą funkcjonalność. Niestety wykrycie błędu w jednym miejscu rzutuje na poprawienie go jedynie w tym miejscu, więc inne (te same) błędy mogą istnieć równolegle w różnych miejscach i nawet, gdy w jednym go zidentyfikujemy i naprawimy, to zawsze jest „to drugie miejsce”. Co gorsze, będziemy święcie przekonani, że błąd został wyeliminowany. Gdy w końcu trafimy do pozostałych miejsc w kodzie, gdzie kod został zduplikowany, to raczej założymy, że nie są wadliwe. Istnieje spore ryzyko, że nie będziemy szukać tego, co już kiedyś naprawiliśmy.

I ostatnim problemem jest sam fakt posiadania metody getAll(). Może bezpośrednio nie jest nigdzie w aplikacji wykorzystywana? Co jeżeli jej użycie zawsze sprowadza się do bycia pewną składową tego, czego tak naprawdę potrzebujemy? To by znaczyło, że mamy więcej problemów z SRP oraz DRY niż nam się może wydawać. To znaczy, że może być sporo miejsc w kodzie, które generują problemy, które właśnie opisałem.

słów kilka i koniec

Ile razy zdarzyło się Wam napisać metodę, której działanie zawsze musiało być uzupełnione? Predykat, który wszędzie był negowany? Metodę zwracającą zbiór, który zawsze był filtrowany?
Jeżeli musicie coś dodać do metody, aby jej wynik był tym, czego potrzebujecie, to pamiętajcie, że dobrze byłoby coś z tym zrobić :)