13 lut 2009

SOLID - Single Responsibility Principle - część 1 - metody

Pisałem już wcześniej o zasadach SOLID, ale pomyślałem, że tak blisko oddają one ducha zwinności designu, że przejdę przez nie po kolei, pokazując jak można zmienić kod napisany przeciętnie w kod napisany dobrze. Co więcej takie zmiany poprawiają czytelność (a więc jakość) kodu ogromnie, a kosztują zwykle bardzo mało. Ubocznym efektem jest łatwiejsze wykrywanie błędów logicznych i duplikowania kodu.

Pierwsza na tapecie jest zasada pojedynczej odpowiedzialności. Jedna metoda odpowiedzialna za jedną funkcję. Jedna klasa reprezentuje jedną rzeczywistość (nie znalazłem lepszego słowa...)
Jako przykład wziąłem metodę z jednego z projektów w których kiedyś brałem udział (zastanawiałem się nad napisaniem kawałka kodu specjalnie pod ten tekst, ale takie przypadki są zwykle nierzeczywiste, więc wziąłem coś co jest z prawdziwego projektu). System realizował workflow, w którym każdy element mógł być na jednym z paru etapów (klasa Stage w kodzie). Ta metoda służyła do przejścia do kolejnego etapu. W czasie aktywowania jednego etapu trzeba było zdezaktywować poprzednio aktywny, ustawić datę aktywowania, deadline dla jego zakończenia, komentarz itp. A oto i kod oryginalnej metody, która to robiła:

/** Activates stage
* SAVES TO DB
*/
void activateStageDeactivatingCurrentlyActive(Stage forActivation)
{
Assert.assertNotNull(forActivation);
Stage forDeactivation = null;

if (forActivation.getDirection() == Stage.IN_DIRECTION)
forDeactivation = findActiveStageIn();
else if (forActivation.getDirection() == Stage.OUT_DIRECTION)
forDeactivation = findActiveStageOut();
else
throw new IllegalStateException("Please supply proper direction");

//if activated stage is not null and is not the same as previous active one
if (!forActivation.sameAs(forDeactivation)){
if (forDeactivation != null) {
forDeactivation.deactivate();
if (forActivation.isRemoved()) {
forActivation.setComment(forDeactivation.getId().toString());
}
}

// fix: #372 update start_date,
// set only when planed started was not set
if (forActivation.getStartDate() == null) {
if (forDeactivation == null){ //when no active stage present before set sysdate
forActivation.setStartDate(
forActivation.getDeadline() == null ? new Date() : forActivation.getDeadline()
);
} else {
forActivation.setStartDate(forDeactivation.getDeadline());
}
}

if (null != forActivation) {
forActivation.activate();
}
}
}

Pierwszy podstawowy warunek dobrego kodu spełniony - mieści się na jednej stronie ;) Generalnie ta metoda nie jest jakaś strasznie długa (na pewno każdy pisał w życiu dłuższe), a pewnie wielu pomyśli, że jest nawet całkiem ok pod względem zawartości i czytelności. Ale ja sądzę, że ta metoda robi jednak o parę rzeczy za dużo, co więcej trudno powiedzieć na pierwszy rzut oka co robi, więc po kolei postaram się doprowadzić ten kod do porządku.

Najpierw usunę nic nie mówiący komentarz i zmienię nazwę metody na samo activateStage. W klasie Stage można napisać, że możliwa jest aktywność tylko jednego, więc w nazwie metody to nie jest potrzebne. Następnie usuwam weryfikację, czy parametr metody nie jest null'em. Takie coś jest na pewno w paru miejscach w kodzie, więc zrobię aspekt, który nie dopuści w ogóle do wywołania tej metody z nullowym parametrem (w ogóle bardzo się zdziwiłem, że w produkcyjnym kodzie jest takie odwołanie do junita...) Moja metoda wygląda więc teraz tak:


void activateStage(Stage forActivation) {
Stage forDeactivation = null;

if (forActivation.getDirection() == Stage.IN_DIRECTION)
forDeactivation = findActiveStageIn();
else if (forActivation.getDirection() == Stage.OUT_DIRECTION)
forDeactivation = findActiveStageOut();
else
throw new IllegalStateException("Please supply proper direction");
.......

Aspekt tu ominę, bo nie o niego tu chodzi. Ma być before execution i tam pewnie wyjątek jeśli null.

Teraz cały fragment odpowiedzialny za wybranie ostatnio aktywnego etapu wywalam do oddzielnej metody (findPreviouslyActiveStage), co mi daje:

void activateStage(Stage forActivation) {
Stage forDeactivation = findPreviouslyActiveStage(forActivation);

//if activated stage is not null and is not the same as previous active one
if (!forActivation.sameAs(forDeactivation)){
if (forDeactivation != null) {
......


Teraz zmieniam warunek otaczający całą resztę tej metody na odwrotny (bez negacji, za to wcześniejszy return), co daje taki kod:

void activateStage(Stage forActivation) {
Stage forDeactivation = findPreviouslyActiveStage(forActivation);
if (forDeactivation.sameAs(forActivation))
return;

if (forDeactivation != null) {
......


Następny etap to ekstrakcja do nowej metody (deactivateStage) fragmentu odpowiedzialnego za deaktywację poprzedniego etapu:

void activateStage(Stage forActivation) {
Stage forDeactivation = findPreviouslyActiveStage(forActivation);
if (forDeactivation.sameAs(forActivation))
return;
deactivateStage(forActivation, forDeactivation);

// fix: #372 update start_date,
// set only when planed started was not set
if (forActivation.getStartDate() == null) {
......

Kolejny etap to ustawienie daty początkowej nowego etapu (jeśli nie była ona ustawiona wcześniej), które również zasługuje
na własną metodę:

void activateStage(Stage forActivation) {
Stage forDeactivation = findPreviouslyActiveStage(forActivation);
if (forDeactivation.sameAs(forActivation))
return;
deactivateStage(forActivation, forDeactivation);
updateStartDate(forActivation, forDeactivation);

if (null != forActivation) {
forActivation.activate();
}
}

No i teraz łatwo mi się zorientować, że forActivation nie może nigdy być null, więc mogę opuścić tę weryfikację przy aktywacji, co daje ostateczną wersję metody:

public void activateStage(Stage forActivation) {
Stage forDeactivation = findPreviouslyActiveStage(forActivation);
if (forDeactivation.equals(forActivation))
return;
deactivateStage(forActivation, forDeactivation);
updateStartDate(forActivation, forDeactivation);
forActivation.activate();
}

No, teraz widać co ta metoda robi. Przede wszystkim sama robi tylko jedną rzecz - aktywuje nowy etap. Cała reszta jest wyciagnięta na do innych metod. Komentarze też nie są tu potrzebne, bo widać co sie dzieje.
Również metody wykorzystywane przez nią zostały poprawione i każda jest odpowiedzialna tylko za jedną rzecz i ew. deleguje inne dalej (już nie będę męczył tu całym refactoringiem...):

private Stage findPreviouslyActiveStage(Stage forActivation) {
switch(forActivation.getDirection()) {
case Stage.IN_DIRECTION:
return findActiveStageIn();
case Stage.OUT_DIRECTION:
return findActiveStageOut();
default:
throw new IllegalStateException("Please supply proper direction");
}
}

private void deactivateStage(Stage forActivation, Stage forDeactivation) {
if (forDeactivation == null)
return;
forDeactivation.deactivate();
setCommentIfStageRemoved(forActivation, forDeactivation);
}

private void setCommentIfStageRemoved(Stage forActivation, Stage forDeactivation) {
if (forActivation.isRemoved())
forActivation.setComment(forDeactivation.getId().toString());
}

private void updateStartDate(Stage forActivation, Stage forDeactivation) {
if (forActivation.getStartDate() != null)
return;

if (forDeactivation != null) {
forActivation.setStartDate(forDeactivation.getDeadline());
return;
}

forActivation.setStartDate(
forActivation.getDeadline() == null ? new Date() : forActivation.getDeadline()
);
}

Pewnie część ludzi woli i tak zapis oryginalny metody :) Do czytania to on może jest lepszy (choć to kwestia opatrzenia się z tak pisanym kodem, którego wszędzie jest pełno), ale zrozumienie tego co robi kod jest bez wątpienia łatwiejsze w zapisie zrefaktoryzowanym. Wystarczy popisać tak parę dni i od razu nawyki się zmieniają. 
Gdy szukamy konkretnego miejsca w kodzie (np. dlatego, że tam jest błąd), łatwiej jest poruszać się po hiearchii małych jednozadaniowych metod, szczególnie jeśli mają łatwo zrozumiałe nazwy.

Ten kod nie jest jeszcze do końca czysty. Trzeba by pewnie pozmieniać parę nazw zmiennych i może przeorganizować logikę, tak by wszystko było na swoim miejscu, ale to już nie jest kwestia pojedynczych odpowiedzialności metod, więc to sobie w tym przykładzie odpuszczam.

Dbanie o zapewnianie pojedynczej odpowiedzialności (a więc unikanie pisania zbyt wiele w jednej metodzie) jest pewnym nawykiem, który trzeba w sobie wyrobić - tak jak unikanie copy-paste, czy za dużo if-else. Oczywiście pisać byle jak jest łatwiej i jasne, że to jest kuszące. Szczególnie gdy koniec projektu blisko, inni nie dbają, stary kodu jest do niczego...  Ale to właśnie te małe wysiłki powodują, że z każdym dniem możesz stać się lepszy w swoim zawodzie. Że to ty możesz zmienić swój projekt, zawstydzić swój zespół, wzbudzić zdrową zazdrość.

Brak komentarzy: