Ten kod mi się nie podoba...

somehow I don't like it

Rok, może dwa lata temu mieliśmy w firmie dyskusję na temat tego, co można zakwalifikować jako defect, a co nie powinno za takowy zostać uznane.
Nie mieliśmy na celu twardej definicji, ponieważ każdy z nas zdawał sobie sprawę, że granica jest dość cienka i subiektywna i najlepiej większą część "klarowania definicji" przerzucić na powolny proces jej naturalnego stabilizowania się w obrębie konkretnego zespołu.

Skupiliśmy się przede wszystkim na tym, co nie powinno zostać nazwane błędem. Pomysł na zrobienie takiego spotkania był wynikiem wielokrotnego pojawiania się "defectów", które nie informowały o niczym, nie dawały żadnych wskazówek i niekiedy nawet, po ich przeczytaniu nie było wiadomo co należy poprawić i co tak bardzo zirytowało osobę przeglądającą kod, że zdecydowała się na ten drastyczny krok.

co błędem kodu nie jest, ...

Stwierdziliśmy zgodnie, że nikt nie ma prawa zgłaszać błędów w stylu: "to mi się nie podoba", "to można zrobić lepiej", "zmień nazwę na lepszą" itp. itd.
Ogólnie rzecz biorąc, każdy błąd powinien jasno wskazywać na to, co jest powodem jego zgłoszenia i zawierać co najmniej jedną propozycję tego, w jaki sposób można go poprawić (co nie jest równoznaczne z tym, że programista musi z nich skorzystać :).
Oczywiście nie znaczy to, że w review mogą pojawić się jedynie tego rodzaju defekty. Definicja jest subiektywną kwestią, z punktu widzenia zespołu, jednak taka decyzja pozwoliła usunąć wiele mało konstruktywnych defectów.

... a co może jednak jest?

Do jednego worka z rzeczami, które błędem nie są, wpadły również takie typu: "nie rozumiem tego", "to jest niejasne", "to CHYBA nie działa tak, jak powinno" itp. Stwierdziliśmy, że to nie defect kodu i to osoba czytająca go (ta, która robi review) jest zobowiązana do tego, aby się dowiedzieć o tym, co się dzieje w kodzie. Ewentualnie, jeżeli programista rzeczywiście wykorzystuje jakieś niewiadomego pochodzenia magiczne konstrukcje, to jest zobligowany do napisania odpowiednich komentarzy w review.
Nasze decyzje były jednogłośne i z pewnością byłby to powód do zadowolenia (ta zgoda), gdyby decyzja nie była błędna. A skąd taka opinia?

co z czytelnością, k#*$a?

Właśnie o czytelności zapomnieliśmy, gdy decydowaliśmy się na takie, a nie inne ustalenia. Sam zastanawiam się, jak mogło się to stać skoro większość zespołów nieustannie przeprowadza refaktoryzację kodu, która przecież na celu ma właśnie tą czytelność podnieść.
A czyż to nie brak tej cechy w kodzie sprawia, że programiści czytający review nie są wstanie stwierdzić o co w kodzie chodzi? Czy to nie jej brak jest powodem tego, że gdybamy i chybamy i musimy zadawać pytania? Czy to nie jest powód, dla którego autor kodu musi pisać odpowiednie komentarze?

Oczywiście nie unikniemy pytań dotyczących kodu, autor nie uniknie jego komentowania w review, jeżeli jednak są one spowodowane brakiem czytelności, to są to błędy, które należy usunąć.

dla zobrazowania

Tak jak pisałem wyżej, stwierdzenie co błędem jest, a co nim nie jest, to subiektywna i niekiedy trudna kwestia i ciężko byłoby stworzyć twardą definicję, która byłaby prawdziwa dla wszystkich. O ile w ogóle jest możliwe skonstruowanie takiej definicji.
Chciałem jedynie uczulić Was, żebyście zwracali uwagę na czytelność kodu, ponieważ jej brak, wcześniej czy później się na Was zemści i dlatego warto traktować jej brak jako błąd. Wprowadzanie ewentualnych poprawek w kodzie, który jest właśnie tworzony, jest mniej kosztowne niż po pewnym czasie. Wszyscy go rozumieją i wszyscy są na bieżąco ze zmianami.

Nie chodzi o to, żeby za każdym razem, gdy nie rozumiecie kodu "sypać" defektami, ponieważ czasami może to rzeczywiście wynikać z braku niezbędnych informacji i wiedzy.
Komentarz w review, który tłumaczy pewne kroki, również nie musi wskazywać na jakikolwiek "problem" z kodem. Może okazać się niezbędny.

Dla zobrazowania tego, o co mi chodzi, poniżej dwa przykłady, które doskonale to pokazują.

if (/** it's look like a magic*/)
Popatrzcie na poniższy kod:

if ($this->_age > 18 && ($this->_nationality !== NATIONALITY::USA || $this->_age > 21))
  $this->goToAParty();

Pewnie po krótkiej chwili domyślicie się, że warunek sprawdza, czy dana osoba może pić alkohol, ale czy nie lepiej (szybciej, czyli wydajniej, czyli oszczędniej) byłoby zobaczyć coś takiego:

if ($this->canDrinkAlkohol())
  $this->goToAParty();

Zrozumienie takiego fragmentu kodu nie wymaga już absolutnie żadnego wysiłku. Zdaję sobie sprawę, że rozmawiamy w tej chwili o oszczędzaniu sekund, ale jeżeli tego typu warunków jest więcej lub stopień ich komplikacji jest większy, to w takich sytuacjach od razu dostrzegamy korzyści tej drobnej zmiany.

if-else-esle if-if-if-else if-else ...
A co powiecie na coś takiego:

if ($request->status === STATUS::SUCCESS)
{
  if (!empty($request->data))
  {
    if (empty($request->errors))
      $this->handleSuccessfulRequest($request);
    else
      $this->handleRequestWithErrors($request, true, false);
  }
  else if (!empty($request->errors))
  {
    $this->handleRequestWithErrors($request, false, false);
  }
  else 
    $this->handleInvalidRequest($request, false);
}
else if ($request->status === STATUS::FAILURE)
{
  $this->handleInvalidRequest($request, true);
}
else 
{
  if (!empty($request->message))
  {
    if (empty($request->errors))
      $this->handleRequestWithErrors($request, false, true);
    else
      $this->handleInvalidRequest($request, false);
  }
  else
    $this->handleInvalidRequest($request, false);
}

Czytelne?

Zamieniając wszystkie odniesienia do atrybutów i układając i porządkując warunki, można uzyskać coś takiego:

if (!$request->isFailed() && (($request->isSucceed() && ($request->hasErrors() || $request->hasData())) || (!$request->isSucceed() && $request->hasMessage() && $request->hasErrors())))
{
    if ($request->isSucceed() && $request->hasData())
        $this->handleSuccessfulRequest($request);
    else
        $this->handleRequestWithErrors($request, $request->hasData(), !$request->isSucceed());
}
else
    $this->handleInvalidRequest($request, $request->isFailed());

Gdy otrzymamy już taki twór możemy wykonać następny krok:

if ($request->isValid())
{
    if ($request->isSuccessful())
        $this->handleSuccessfulRequest($request);
    else
        $this->handleRequestWithErrors($request, $request->hasData(), !$request->isSucceed());
}
else
    $this->handleInvalidRequest($request, $request->isFailed());

... i:

if ($request->isValid())
    $this->handleValidRequest($request);
else
    $this->handleInvalidRequest($request, $request->isFailed());

... już lepiej :)

Oczywiście można myśleć o dalszym usprawnianiu (zwróćcie uwagę, że metody obsługujące żądanie przyjmują wartości, które są rezultatem wywoływania metod na obiekcie $request, który zawsze jest do nich przekazywany), ale to już sobie odpuszczę:)

na zakończenie

Nie chciałbym, abyście po przeczytanie tego wpisu zaczęli zgłaszać błąd za błędem, jak tylko zobaczycie coś, co Wam nie pasuje, nie można popadać w skrajności.
Jednak następnym razem, gdy spojrzycie na kod i zaczniecie się zastanawiać "o co w tym wszystkim k#$%a chodzi?" pomyślcie, czy przypadkiem pytanie, które zrodziło się w Waszej głowie nie jest wynikiem słabej jakości kodu.

I pamiętajcie, że jeżeli raz "przymkniecie oko", to takie "cudowne rozwiązania" mają tendencję do wzrostu :)