24 lut 2009

SOLID - Open-Closed Principle

W oryginalnej wersji zasada otwartości-zamknięcia brzmi tak: Elementy oprogramowania (klasy, moduły, funkcje) powinny być otwarte na rozszerzanie ale zamknięte na zmiany.

Hm, zwykle gdy chcemy dodać jakąś funkcjonalność po prostu dopisujemy odpowiedni kod. Weźmy przykład:

public class User {
private String login;
private String password;

private UserRepository repo;

public void delete () {
repo.delete(this);
}

public User findByName(String name) {
return repo.findByName(name);
}
}

public class UserRepository {
private EntityManager em;

public void delete(User user) {
em.remove(user);
}

public User findByName(String name) {
return (User) em.createQuery("from User u where u.name=:name")
.setParameter("name", name)
.getSingleResult();
}
}

Klasy proste, śliczne (oczywiście troszkę uproszczone :)). I nagle przychodzi wymaganie: użytkownik który ma uprawnienia administratora ma mieć flagę deleted która oznacza, że tak naprawdę nie ma być usuwany, tylko zaznaczony jako usunięty i nie prezentowany już potem w GUI. No to pierwsze podejście pewnie będzie takie:

public class User {
private String login;
private String password;
private boolean admin;
private boolean deleted;

private UserRepository repo;

public void delete () {
repo.delete(this);
}

public User findByName(String name) {
return repo.findByName(name);
}
}

public class UserRepository {
private EntityManager em;

public User findByName(String name) {
return (User) em.createQuery("from User u where u.name=:name and deleted=false")
.setParameter("name", name)
.getSingleResult();
}

public void delete(User user) {
if (user.isAdmin()) {
user.setDeleted(true);
em.persist(user);
} else {
em.remove(user);
}
}
}

No, gotowe! Tylko, czy to jest dobry design? Już if w metodzie usuwającej wskazuje, że dzieje się tam więcej niż jedna rzecz. Co więcej, zasada pojedynczej odpowiedzialności dużo nam tu nie pomoże, możemy co jakwyżej osiągnąć coś takiego:

public class User {
private String login;
private String password;
private boolean admin;
private boolean deleted;

private UserRepository repo;

public void delete () {
if (! admin)
repo.deleteNonAdmin(this);
else {
repo.deleteAdmin(this);
}
}

public User findByName(String name) {
return repo.findByName(name);
}
}

public class UserRepository {
private EntityManager em;

public User findByName(String name) {
return (User) em.createQuery("from User u where u.name=:name and deleted=false")
.setParameter("name", name)
.getSingleResult();
}

public void deleteNonAdmin(User user) {
em.remove(user);
}

public void deleteAdmin(User user) {
user.setDeleted(true);
em.persist(user);
}
}

Zwiększyliśmy trochę ilość kodu (twój manager już jest zadowolony - kolejna wskaźnik postępu zwiększony!), może trochę lepiej widać na czym polega różnica między usuwaniem administratora i nie-administratora, ale tak naprawdę kod wiele się nie poprawił. Jeśli będą dochodziły kolejne wymagania jak to, nasza metoda delete zmieni się wkrótce w wielki ciąg if-else. I tu właśnie jest miejsce do zastosowania zasady otwartości-zamknięcia. 
Kod powinien być otwarty na rozszerzanie a zamknięty na modyfikację.


Co to znaczy w praktyce?
Dziedziczenie.

Jeśli przypomnimy sobie, że w językach obiektowych możemy dziedziczyć, okaże się, że ten kod jest jeszcze do uratowania :)

public class User {
protected String login;
protected String password;

protected UserRepository repo;

public User findByName(String name) {
return repo.findByName(name);
}

public void delete() {
repo.delete(this);
}
}

public class AdminUser extends User {
boolean admin;
private boolean deleted;
}


public class UserRepository {
protected EntityManager em;

public User findByName(String name) {
return (User) em.createQuery("from User u where u.name=:name")
.setParameter("name", name).getSingleResult();
}

public void delete(User user) {
em.remove(user);
}
}

public class AdminUserRepository extends UserRepository {
public User findByName(String name) {
return (User) em.createQuery("from User u where u.name=:name and deleted=false")
.setParameter("name", name)
.getSingleResult();
}

public void delete(AdminUser user) {
user.setDeleted(true);
em.persist(user);
}
}

Teraz if-else w ogóle nie istnieje. Kod wołający metody findByName i delete dalej nie musi wiedzieć, że dana instancja klasy User jest administratorem, ale nowa struktura kodu wyeliminowała potrzebę tworzenia warunków (zwykle było by ich więcej niż tylko w jednym miejscu, więc jeśli w kodzie masz gdzieś ten sam warunek sprawdzany parę razy w różnych metodach, to prawdopodobnie powinieneś zastanowić się nad dziedziczeniem). Kod jest teraz dużo bardziej przejrzysty (np. od razu wiadomo gdzie szukać jeśli coś jest nie tak z administratorem). 

Ta zasada wydaje się zupełnie oczywista. Nie trzeba doktoratu z informatyki, żeby wpaść na takie rozwiązanie. Jednak jakże często dodajemy funkcjonalności do kodu bez większego zastanowienia. Po prostu dodajemy kod. Pilnowanie zasady otwartości-zamknięcia jest kolejnym sposobem na nie zwiększanie długu technicznego (nie pogorszanie jakości kodu) wraz z dodawaniem funkcjonalności.

Pisanie i projektowanie aplikacji poprzez stosowanie TDD powoduje, że czas na zastanowienie się nad najlepszym rozwiązaniem staje się nieodłączną częścią procesu rozwoju oprogramowania wykonywaną z resztą co parę minut, co oczywiście ma bezpośredni wpływ na jakość kodu który po sobie pozostawiamy. Istnienie testów powoduje, że nie mamy obaw związanych z wprowadzaniem większych zmian do oprogramowania - np. tak jak w powyższym przykładzie z wymianą warunków na dziedziczenie.

Od tego w jaki sposób wprowadzamy rozszerzenia do naszego projektu zależy bardzo dużo. Czas developmentu, ryzyko błędu, może nawet w końcu przepisywanie projektu. Wszystko to sprowadza się do marnowania dużych ilości pieniędzy naszego pracodawcy / klienta, może nawet osłabić jego konkurencyjność, a przecież nikt nie dał nam do tego prawa. I jeśli Twój project manager czy klient ciśnie Cię terminami, to może zamiast bezwolnie poddawać się tej presji lepiej dokładnie mu to wytłumaczyć. To nie jest Twoja rola, żeby podejmować decyzje o stałym obniżaniu jakości kodu przez byle jakie wrzucanie nowych linii do istniejącego kodu, więc zrzuć to na osoby które takie decyzje są władne podjąć. Niech twój PM podpisze się własną krwią pod zdaniem: 
Tak, chcę doprowadzić do obniżenia jakości oprogramowania. Bardziej niż na jakości zależy mi na terminach. Jestem skłonny raczej w przyszłości mieć ciągłe problemy z ukańczaniem funkcjonalności i błędami niż zmienić termin tego releasu.
Jeśli to zrobi (co niestety się zdarza), przynajmniej będziesz miał czyste sumienie. I nikt nie zarzuci ci braku profesjonalizmu i nieodpowiedzialności.

19 lut 2009

SOLID - SRP część 2 - klasy

Ostatnio pisałem o Regule Pojedynczej Odpowiedzialności skupiając się na metodach. Ale ta reguła dotyczy w równym stopniu strukturyzowania klas. Tym razem będzie mniej kodu, bo skupię się tylko na sygnaturach pokazując o co chodzi.

Reguła Pojedynczej Odpowiedzialności brzmi w tym przypadku tak: powinien być dokładnie jeden powód do dokonywania zmian w klasie. Oznacza to, że kod powinien mieć taką strukturę, żeby każda klasa miała dokładnie jedną odpowiedzialność: np. wyświetlanie komponentu, komunikacja z bazą danych, logika biznesowa.

Najbardziej klasycznym przykładem zastosowania tej reguły jest wzorzec MVC. Oddzielenie od siebie danych, mechanizmu wyświetlania i obsługa operacji UI.

To przecież jasne! Każdy to wie, nie?

No to zobaczmy czy napewno. Mamy sobie taką klasę:

@Stateless
@Local
public class UserManagerBean {

@PersistenceContext(type = PersistenceContextType.TRANSACTION)
private EntityManager em;
private LoginManager loginManager;

public User findUserByName(String name) {
return (User) em.createQuery("from User u where u.name=:name")
.setParameter("name", name).getSingleResult();
}

public boolean isUserValid(User user) {
if (user.getNumWrongLogins() > 3
&& (System.currentTimeMillis() - user.getLastLoginTrial() < 5000))
user.block();

if (user.isBlocked() || user.isAccountExpired())
return false;
return true;
}

public String login(User user) {
boolean result = loginManager.login(user.getName(), user.getPassword());
if (result)
return "You logged in successfully!";
return "Logging in failed";
}
}

Ten kod na pierwszy rzut oka wygląda ok. Gdyby to był poprzedni wpis, przyczepiłbym się tylko metody isUserValid, bo robi dwie rzeczy zamiast jednej (weryfikuje czy blokować Usera i ew. blokuje, oraz sprawdza czy User może się logować). Ale z punktu widzenia Reguły Pojedynczej Odpowiedzialności klas ten kod jest zupełnie do niczego... a dokładniej to do wszystkiego :)  (jak z resztą większość EJB na tym świecie, ponieważ ten model promuje takie programowanie). Ta klasa w jednej metodzie komunikuje się z bazą danych, w drugiej zawiera logikę, w trzeciej zwraca tekst do wyświetlenia w UI. Brrrrr. Straszne.

W przykładzie klasy, która ma 35 linii może jeszcze to tak nie kłuje w oczy, ale taka klasa mogła by mieć 30 różnych metod i wtedy przestaje być jasne za co taka klasa jest odpowiedzialna, co gdzie należy zmienić jeśli chce się wprowadzić nową funkcjonalność lub zmodyfikować istniejącą. Przy dużej liczbie takich komponentów aplikacja przestaje być utrzymywalna. Odpowiedzialności mieszają się ze sobą, obiekty wołają nawzajem różne swoje metody tworząc sieć zależności i oczekiwań. W w dużych aplikacjach ze skomplikowanymi procesami brak poprawnego i dokładnego rozdzielenia odpowiedzialności powoduje, że zmiany w jednej metodzie powodują często powstawanie błędów w wielu innych miejscach aplikacji.

Jak więc należało by ten problem rozwiązać? Ja proponowałbym kod bazodanowy wyprowadzić do odpowiedniego modułu/warstwy (DAO, repozytorium, etc.) a dostęp do niego zrealizować np. jako metodę samego obiektu User (np. a'la Grails'owy GORM: User.findByName(name)). Weryfikacja poprawności użytkownika albo do obiektu walidacji, albo do klasy User (jako User.isValid()). Kod zwracający treść do wyświetlenia przerzucić na stronę wyświetlającą (albo jako tag czy fragment) a samo wywołanie logowania (linia wołająca obiekt loginManager) jest już odpowiednie dla kontrolera UI. 
I w ten sposób... nie ma już więcej UserManagerBean'a :) (tak, to taki sprytny sposób podprogowego przekazania myśli, że EJB nie powinny być wykorzystywane do metod które nie korzystają bezpośrednio z transakcyjności, bezpieczeństwa czy remotingu)
Jeśli będzie zmiana w strukturze danych w bazie albo zmieni się wymaganie co do zapytania zwracającego użytkownika, jest jasne gdzie tego szukać - w klasie reprezentującej repozytorium obiektów User. Jeśli będziemy chcieli dodać i18n, będzie to w miejscu odpowiedzialnym za wyświetlanie. Jeśli będzie trzeba zwiększyć dostępną liczbę nieudanych prób logowania Użytkownika, to też łatwo będzie ustalić gdzie jest odpowiedni kod.

Przy definiowaniu przynależności metod do klas można wykorzystać heurystykę częstotliwości odwołań do różnych klas w ramach metody. Jeśli w jakiejś metodzie większość wywołań metod dotyczy jednej klasy, to z dużym prawdopodobieństwem ta metoda powinna w tej klasie się znaleźć (zmniejszając ilość kodu o odwołania do obiektu i poprawiając jego logiczne rozłożenie).
Jeśli dbamy o to, by metody były krótkie, tych odwołań nie będzie dużo i łatwiej o dobry (sprawny) design, stąd jeszcze jeden powód do pilnowania pojedynczej odpowiedzialności metod. W ten sposób dużo łatwiej jest osiągnąć Święty Graal projektowania obiektowego: High Cohesion with Loose Coupling

Choć w sumie Zasada Pojedynczej Odpowiedzialności dotyczy zupełnie czego innego w przypadku metod niż w przypadku klas, wykorzystywanie jej w obu (rozdzielone małe metody o jednej odpowiedzialności umieszczone w odpowiednich klasach) ma niebagatelny wpływ na jakość naszego kodu a więc na łatwość jego zrozumienia przez innych oraz jego utrzymywalność. 

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ść.

Jakość nie ma znaczenia

Ostatnio wśród speców od programowania rozgorzała dyskusja nad jakością kodu/designu versus czas rozwijania.  Rozpoczął ją Joel Spolsky wypuszczając podcast w którym pada zdanie: 
And I find, sadly, to be completely honest with everybody listening, quality really doesn't matter that much, in the big scheme of things...
W tym samym podcast'cie Joel mówi tak:
Last week I was listening to a podcast on Hanselminutes, with Robert Martin talking about the SOLID principles. (...)It's object-oriented design, and they're calling it agile design, which it really, really isn't. It's principles for how to design your classes, and how they should work. And, when I was listening to them, they all sounded to me like extremely bureaucratic programming that came from the mind of somebody that has not written a lot of code, frankly.
No więc co znaczy agile w kontekście oprogramowania? Mniej więcej to samo co w kontekście prowadzenia projektów: reagowanie na zmiany. I z tego punktu widzenia zasady SOLID (o których było parę postów temu) są bardzo zwinne, bo pozwalają z dużo większą łatwością dodawać nowe funkcje do systemu i modyfikować istniejące. Nie bardzo wiem z czym można tu dyskutować. Programowanie które nie jest w tym sensie "biurokratyczne" to programowanie chaotyczne. 
A mówienie o Robercie C. Martinie, Martinie Fowlerze, Kencie Becku i innych ludziach stojących za zwinnością w sensie technologicznym, że nie napisali wiele kodu jest po prostu żenujące.


Pierwszy cytat dotyczący jakości to jasny dowód na to, że jego autor jest PMem...  Dba o TEN projekt, o TĘ kasę, o TE terminy. Dług techniczny zaciągnięty w tym czasie nie jest dla niego ważny, bo przecież później się to posprząta.  Z resztą ten cytat mówi sam za siebie:
The way real software works is that you create these very imperfect things, and they work great. They really do. And then you have a little problem, and you go and you fix the little problem, because it's code, and you have an editor, and you edit it. 
Taaak. Przykładów oprogramowania, które works great jest faktycznie wiele... 
Dług techniczny nazbierany przez lata powoduje, że właśnie nie da się po prostu otworzyć edytora i poprawić kodu. Z resztą zwrot little problem też pokazuje nastawienie pana Spolsky'ego. No ale on robi soft do rejestrowania błędów, więc zależy mu na tym, żeby było ich jak najwięcej - wtedy więcej kopii sprzeda :)

Straszne, że ktoś tak wpływowy w świecie oprogramowania pisze takie rzeczy.


Takie krótkowzroczne spojrzenie jest ze stratą dla wszystkich. Programiści się frustrują i nie chcą potem pracować z takim kodem. Klient dostaje coraz dłuższe terminy (a więc i większe kwoty), albo te co są są niedotrzymywane. PM ma zmarnowane życie prywatne i wrzody żołądka (choć na to to akurat sobie zasłużył :)). 
Jedną z podstawowych wartości ruchu agile jest transparentność i szczerość. I dotyczy to procesów (wiadomo kto, co i kiedy robi), efektów (ukończonych zadań), kosztów. Ale mało kto mówi, że dotyczy to również kodu. I to nie tylko w zespole programistów. Kto informuje swojego klienta o powstającym na skutek jego presji długu technicznym? Kto mówi, że żeby robić release'y na czas wprowadził do systemu taki shit, że robienie nowych funkcjonalności zajmie 20% czasu więcej (a więc 20% więcej będzie kosztować, co może przekona klienta)?
Odpowiedzialny zaspół zgłasza swojemu Product Owner'owi (a ten klientowi) informacje o wpływie jego decyzji na jakość. Jakość jest wartością biznesową a nie techniczną i jako taka powinna być uświadamiana klientowi. Jeśli twój PM nie pozwala Ci dbać odpowiednio o jakość (ciśnie terminami, siedzisz po godzinach, nie testujesz kodu itp.) to działa na szkodę klienta (to on ostatecznie dostanie shit) i na szkodę Twojej firmy. Warto więc uświadamiać w tym zakresie osoby niezaangażowane bezpośrednio w projekt, a mogące podejmować decyzje.

6 lut 2009

Ekstremalna obiektowość

Kto mnie zna, wie, że jestem stuknięty jeśli chodzi o obiektowość. Tak od paru lat. Z każdym projektem w którym uczestniczę (ew. który przeglądam...) coraz bardziej. Od czasu kiedy stworzyłem parę lat temu klasę-potwora - półtoratysiąca linii, z których z 500 było w jednej metodzie i nie dane mi było tej Augiaszowej stajni posprzątać, z wszystkich rzeczy w programowaniu dbam najbardziej o zachowywanie zasady pojedynczej odpowiedzialności. Jeśli miewam w nocy jakieś koszmary, to dotyczą one zawsze refactoringu tej klasy :)

W związku z tym moim małym zboczeniem, z wielką ciekawością przeczytałem (parokrotnie) jeden z rozdziałów w książce, którą aktualnie mam na tapecie - The Thoughtworks Anthology. Rozdział nosi tytuł Object Calisthenics (można go gdzieś w necie znaleźć jako .doc). Jeff Bay proponuje w nim 9 reguł dotyczących tworzenia kodu (przykłady są w javie, ale większość reguł odnosić się może do w miarę dowolnego języka, szczególnie OO). Są to
  • Tylko jeden poziom zagłębienia na metodę
  • Nie używaj słowa kluczowego else
  • Opakowuj wszystkie prymitywy i Stringi (w klasy o specyficznej dla zastosowania nazwie)
  • Używaj tylko jednej kropki na linię
  • Nie skracaj nazw
  • Pilnuj wszystkie encje by były małe
  • Nie używaj klas o więcej niż dwóch polach
  • Klasa której polem jest kolekcja nie powinna mieć żadnych innych pól (opakowywanie kolekcji w klasy specyficzne dla kontekstu wykorzystania)
  • Nie używaj getterów/setterów/własności
Wow - pomyślałem. To jak tu w ogóle pisać? Ale Mr. Bay jest w swoim tłumaczeniu dość przekonujący. Wolałbym co prawda więcej przykładów w artykule, ale już samo przedstawione przez niego rationale przekonuje mnie do większości z tych stwierdzeń. Co więcej, większość z nich i tak bardzo często stosuję (opakowywanie kolekcji i prymitywów, jeden poziom zagłębienia, nieskracanie nazw), ale autor przekonuje, żeby stosować te reguły wręcz fanatycznie. 

Parę rzeczy jest pewnych: 
  • taki kod jest bardziej obiektowy (podział odpowiedzialności, właściwe opakowywanie, itp.)
  • taki kod jest bardziej testowalny (małe metody, proste testy - taki design sam się prosi o TDD)
  • taki kod jest bardziej reużywalny (łatwiej znaleźć wiele zastosowań dla małej metody o dobrze określonej odpowiedzialności niż dla metody, która robi pięć rzeczy)
  • taki kod sam się dokumentuje (krótkie jednoznaczne nazwy metod powodują, że kod się czyta prawie jak zwykły tekst)
  • trudniej zrobić a łatwiej wykryć błąd w metodzie o 5 liniach niż w metodzie o 500 (to akurat wiem dobrze)
Może więc jednak warto naciskać na obiektowość i doprowadzać ją do ekstremum. 

Mimo, że generalnie artykuł Jeffa Bay'a jest bardzo blisko moich własnych praktyk programistycznych, wydaje mi się on zbyt ekstremalny. Póki co do niektórych reguł (a raczej ich fanatycznego stosowania) jestem trochę sceptycznie nastawiony, szczególnie jesli wziąć pod uwagę, że nasz kod zależy czasem od zewnętrznych frameworków/bibliotek które nie pozwalają nam na pełną elastyczność (jak na przykład pogodzić takie rozdrobnienie, brak getterów i setterów itp. z np. mapowaniem hibernate'owym). 
Tak czy siak autor zachęca do zrobienia ćwiczenia: 1000 linii kodu napisanego trzymając się tych reguł. A że w poniedziałek i wtorek mam szkolenie poza Warszawą, przynajmniej połowę z tego wyrobię :)

2 lut 2009

Narty

Skończyłem właśnie ferie zimowe w czasie których jeździłem trochę na nartach i już pierwszego dnia na stoku miałem następujące skojarzenie: 
Co ma wspólnego stok narciarski z R&D? Większość ludzi na stoku myśli, że umie jeździć jak zasuwa na krechę z nogami rozstawionymi jakby drzewo z obu stron na raz chcieli wyminąć, kijkami na boki i wrzaskiem 'uwaaaagaaa'. W naszej działce bieg do zakończenia projektu wygląda podobnie - ma być szybko, więc często jest byle jak.
Ludzie którzy jeżdżą na nartach zwykle są zadowoleni jak umieją zjechać z górki bez przewrócenia się i to jest koniec ich ambicji. I dlatego trudniejsze trasy są puste. Bardziej wymagający śnieg powoduje, że wszyscy leżą, a następnego dnia stok jest pusty. Brak umiejętności wychodzi od razu gdy warunki nie są idealne.
Programiści mogą być skazani na ten sam los. Jeśli nie będziemy się rozwijać, uczyć, ciągle poprawiać, dyskutować z innymi, patrzeć na cudzy kod, jakiekolwiek zawirowanie w projekcie, presja czasu czy trudne zagadnienie, od razu powodują porażkę.
Znane z metodyki lean pojęcie kaizen (continous improvement) dotyczy dokładnie tego: ciągły rozwój, zwiększanie swojej wiedzy, doświadczenia. Idealnie wpasowuje się to w parę idei z książki Pragmatic Programmer, np. co roku naucz się nowego języka programowania - nie? 

Ja w na ten rok zaplanowałem uaktualnienie wiedzy o Springu, dobre poznanie Scali (może jeszcze Clojure?) i hiszpański (mało programistyczne zagadnienie... :) )

A ty czego zamierzasz nauczyć się w najbliższym czasie?


PS. W czasie ferii byłem na niekonferencji Cooluary w Krakowie. Atmosfera fajna,  ludzie konkretni i z wiedzą, dyskusje ciekawe i cena przystępna. Mówiąc krótko polecam wszysktim na przyszły rok!