Fekofile

Dostałem dzisiaj ciekawego maila. Zawierał poniższy tekst opisujący, jak ktoś wysłał maila do swoich współpracowników o refaktoringu, który zrobił:

Po tym, jak wysłałem ten e-mail do moich kolegów, spotkałem się z następującą reakcją; 1 członek zespołu uważał, że to było lepsze niż poprzednio i 2 osoby uważały, że to było im trudniej zrozumieć. Z tych 2 osób, które uważały to za gorsze; 1 uważa, że powinienem cofnąć swoje zmiany, a druga jest w stanie znieść te zmiany, żebym już tylko się zamknął :-)

---

Poniższy tekst jest luźnym tłumaczeniem wpisu bloga Roberta Cecila "Wujka Boba" Martina z dnia 20 stycznia 2012 ze strony:

https://blog.cleancoder.com/uncle-bob/2012/01/20/Fecophiles.html

Proszę o komentarze, jeżeli ta luźność jest zbyt daleko posunięta.

---

Tu jest e-mail, który im wysłał, pokazujący kod przed refaktoringiem i po refaktoringu.

Temat: FW: Refaktoring miesiąca


Cos pominąłem? Czy właśnie udało mi się zejść z 31 linii kodu do 2 linii kodu?

------- przed zmianami ------
public static string GetLtsCode(IventoryBinItem item)
{
    string ltsCode = null;
    if (!string.IsNullOrEmpty(item.ParentItemNo)) //child item
    {

        ltsCode = "PzT";
        var isNfoOrDisc = item.IsNfoItem || item.IsDiscontinuedItem;
        //if (isNfoOrDisc && item.ParentIsNfoOrDiscontinuedItem ||
        //    !isNfoOrDisc && !item.ParentIsNfoOrDiscontinuedItem)
        if (!item.ParentIsNfoOrDiscontinuedItem)
        {
            ltsCode = "PzT";
        }
        //else if (item.ParentIsNfoOrDiscontinuedItem && !isNfoOrDisc)
        else if (item.ParentIsNfoOrDiscontinuedItem) //always capture demand for children of nfo (april19 change)
        //and childitem is not, then mark it as regular, otehrwise both PzT
        {
            ltsCode = null;
        }
    }
    else //parent
    {
        ltsCode = null;
        if (item.IsNfoItem || item.IsDiscontinuedItem)
        {
            //april 19 change
            //ltsCode = "PzT";
            ltsCode = null;
        }

    }

    return ltsCode;
}
-----------------

---- po zmianach ------
public static string GetLtsCode(IventoryBinItem item)
{
    bool isPzT = item != null
              && !item.ParentItemNo.IsNullEmptyOrWhiteSpace()
              && !item.ParentIsNfoOrDiscontinuedItem;
    return isPzT ? "PzT" : null;
}
------------------

---- I później, po małym refaktoringu ----
public static string GetLtsCode(IventoryBinItem item)
{
    return IsPzT(item) ? "PzT" : null;
}

private static bool IsPzT(IventoryBinItem item)
{
    return item != null
       && !item.ParentItemNo.IsNullEmptyOrWhiteSpace()
       && !item.ParentIsNfoOrDiscontinuedItem;
}
------------------

Pozwól mi wyrazić się jasno. Pierwszy kod to gówno. Wielkie gówno. Mam na myśli naprawdę śmierdzącą, rozwodnioną i brązową kupę kupy. Wygląda, jakby wyszła z naprawdę chorego psa.

Byłem zdumiony, że ktokolwiek mógłby uważać, że to gówno było lepsze od całkiem niezłego refaktoringu. Wydaje się, że niektórzy ludzie po prostu lubią wąchać kupę. Nazywam ich Fekofilami.

Pomyślałem sobie, że może mają coś nie w porządku z nosami. Koniec końców, jeżeli na co dzień żyjesz w gównie, po jakimś czasie możesz już po prostu tego nie czuć. Więc zdecydowałem się wepchnąć ich nosy w tę śmierdzącą kupę poprzez prześledzenie tego zapachu w każdym z jego intensywnych i gryzących detali. Więc, wysłałem z powrotem do mojego przyjaciela następujący e-mail.

---

Przejdźmy przez cały ten poprzedni kod, naprawdę obwąchując go dookoła!

1. Zaczynamy poprzez zainicjowanie zmiennej ltsCode nullem. W porządku.
2. Wtedy napotykamy podwójne zaprzeczenie (jeśli nie null/pusty) i dwukierunkowość (item.ParentItemNo). Potrzeba komentarza, żeby to zrozumieć. Wydaje się, że jeżeli ParentItemNo elementu będzie nullem lub puste, to znaczy, że element nie ma rodzica. Jeżeli dodamy zaprzeczenie, wyrażenie if zdaje się mówić: "jeżeli to jest dziecko". Komentarz stara się o tym powiedzieć, ale gramatyka kodu jest zła. Byłoby ładniej, gdyby wyrażenie if miało postać if(isChild(item)) lub if(item.isChild()).
3. W ciele wyrażenia if jesteśmy dzieckiem, więc ustawiamy ltsCode na PzT.
4. Ignorujemy zakomentowany kod.
5. Ustawiamy zmienną isNfoOrDisc na interesujące wyrażenie. Co dziwne, nie widzę żadnego miejsca, w którym ta zmienna byłaby używana. Kiedyś była używana w zakomentowanym kodzie, więc może ktoś po prostu zapomniał zakomentować także zmiennej, co?
6. Zaprzeczenie w wyrażeniu if (łatwo przegapić ten fakt).
7. Wyrażenie if sprawdza, czy rodzic nie jest nfo lub nie jest już kontynuowany. Jeżeli tak, ustaw ltsCode na PzT. Ale przecież ltsCode jest już ustawione na PzT. Może ktoś zapomniał zakomentować całego tego wyrażenia if. &ltA fe!&gt
8. Jeszcze więcej zakomentowanego kodu. Fuj.
9. Aaa! A teraz klauzula else "rodzica Nie nfo lub niekontynuowanego" wyrażenia if, którego zaprzeczenie wyrażenia boolean sprawdzaliśmy wcześniej. Czy programista nie wie, jak działa if/else? Dlaczego do diabła on sprawdza tautologię? Przecież, w klauzuli else, my już WIEMY, że rodzic jest nfo lub nie jest kontynuowany. Do licha.
10. No i w klauzuli else ustawiamy ltsCode na null. Czekaj, czekaj ... Nie ustawialiśmy go kiedyś wcześniej już na null?
11. (uff, uff, uff) ok, doszliśmy do else. Co my tu mamy? To jest else z podwójnego przeczenia wyrażenia if z punktu 2. wcześniej. Aaa, więc to tu mamy element bez rodzica. Ten komentarz //parent jest dziwny. Czy to oznacza, że element jest rodzicem? Skąd to mamy wiedzieć? Wszystko, co wiem, to to, że element nie posiada nie nullowego lub pustego numeru elementu rodzica. A więc co jest z komentarzem //parent? To kłamstwo albo jakiś kiepski żart. Wydaje mi się, że komentarze powinny być wykomentowane. (wrrrr).
12. Ustawiamy ltsCode na null. Hmmmm. zobaczmy. Jaką miało wartość ltsCode, zanim ustawiliśmy je na null? Wszak, wydaje mi się, że już jest nullem! Czy jest jakaś ścieżka wśród tego okropnego programistycznego gniazda węży, która mogłaby ustawić ltsCode na cokolwiek różnego od nulla? Nie ma!
13. a teraz ostateczny cios! Kolejne wyrażenie if, które sprawdza, czy element jest nfo czy nie kontynowany. I co to wyrażenie if robi, jeżeli tak jest? Ano, ustawia ltsCode na ... NULL. Oczywiście! NULL. Genialne!

Podsumowując. Ten kod to śmietnik. W całym znaczeniu tego słowa, bezdyskusyjny, prawdziwy burdel. Jest pełen niedokładnych i dwuznacznych komentarzy, niepotrzebnych zmiennych, bezcelowych wyrażeń if i naprawdę zawiłej logiki. A jaki był cel? Cel był taki:

zwróć PzT, jeżeli element istnieje i ma rodzica, który nie jest nfo lub jest niekontynuowany. W przeciwnym razie zwróć nulla.

LUB, ujmując to w prostym kodzie

 public static string GetLtsCode(IventoryBinItem item) {
  return IsPzT(item) ? "PzT" : null;
 }

 private static bool IsPzT(IventoryBinItem item) {
  return item != null
    && item.HasParent()
    && item.ParentIsNotnfoOrDiscontinuedItem;
 }

Wstydzę się każdego, kto uważa, że oryginalny kod jest łatwiejszy do zrozumienia.

---

Nie wiem, czy to do nich doszło. Ale nawet, jeśli po raz pierwszy uświadomili sobie, że w ich domu śmierdzi, to jest to kropla w morzu. Jest jeszcze wielu, wielu fekofili na tym świecie, którzy muszą cofnąć się krok w tył i wziąć głęboki wdech nosem.

---

Powyższy tekst jest luźnym tłumaczeniem wpisu bloga Roberta Cecila "Wujka Boba" Martina z dnia 20 stycznia 2012 ze strony:

https://blog.cleancoder.com/uncle-bob/2012/01/20/Fecophiles.html

Proszę o komentarze, jeżeli ta luźność jest zbyt daleko posunięta.

---