Blog webdeveloperski Patryk yarpo Jar

Komentarz – martwy kod, o który trzeba dbać

Autor wiadomości Październik 24, 2011

Każdemu początkującemu programiście nauczyciele wmawiają (ucząc jak pisać komentarze), że dobry kod posiada dużo komentarzy.

BZZZZDUURRRRAAA!

Dobry kod nie wymaga komentarzy* (lub też ich ilość jest ograniczona do absolutnego minimum). Zły kod, jest komentarzolubmy, a czasem komentarzowymagający.

* - Mowa o nowoczesnych językach wysokiego poziomu (Java, PHP, C# itp.).

Przykład złego komentarza:

// zmienna trzymajaca liczbe uczniow w klasie
var x = 17;
Czy nie lepiej jest zrobić np.:
var liczebnoscKlasy = 17;

  • Czy jest krócej?
  • Tak.
  • Czy mniej czytelnie?
  • NIE. W sumie bardziej czytelnie, bo przecież nie będziesz komentować każdego wystąpienia zmiennej `x'. Zmienna `liczebnoscKlasy' sama się tłumaczy.

Inny przykład złego komentarza:

// wyświetl komunikat z prośbą o podanie imienia
printf("Proszę podaj imię");
// odczytywanie danych ze standardowego wejścia
scanf (%s, str);
// przechodzę w pętli po wszystkich wczytanych znakach
for(int i = 0; i < strlen(str); i++)
{
     // do każdej pozycji tablicy odejmuję wartość 32
     str[i] = str[i] - 32;
}
// wyświetlam dane na standardowe wyjście
puts(str);

Ktoś powie "co nie tak jest w tym kodzie?!" Wszystko ładnie okomentowane, nie ma linii, która nie byłaby dokładnie wytłumaczona... No tak, ale niezbyt te komentarze rozsądne. Podzielić je można na dwie grupy:

  • zbędne (zbyt oczywiste)
  • nieprawidłowe (nie mówią co to nam daje, ale co kod robi - zaraz wytłumaczę)

 Jak poprawić powyższy kod?

Powyższy kod można łatwo napisać ładniej i czytelniej. Dodatkowo krócej, a i zabezpieczając kod przed nieprawidłowym działaniem:

printf("Proszę podaj imię");
scanf ("%s", imie);
int n = strlen(imie);
while (n--)
{
    imie[i] = toupper(imie[i]);
}
puts(imie);
No i co jest lepiej?

Krócej? Tak.

Są komentarze? Nie. Ale czy czytelność spadła? IMO Nie.

Zabezpieczenia?

Wcześniej od każdego znaku odejmowana była wartość 32. Jest to stary sposób na powiększenie litery (ma to związek ze znakami ASCII, gdzie wielkie litery mają numer o 32 mniejszy od małych). Wykorzystanie `toupper' (<ctype.h>) pozwala:

  1. skrócić nasz kod
  2. zabezpiecza przed sytuacjami, kiedy mamy już wielką literę (w poprzednim przypadku brakowało tego warunku)
  3. zlikwidowaliśmy "magiczną liczbę" 32 (Czy patrząc na ten kod pierwszy raz wiedziałeś, co ona robi?). Zamiast stosowania takich nic nie mówiących liczb lepiej jest dać stałą, której nazwa jasno określa jej przeznaczenie.
  4. usunąć poprzedni komentarz, który nic nie mówił o tym, co chcemy uzyskać. Informacja o tym, że chcemy zmienić wartość była zapisana w kodzie, nie trzeba było jej dublować.

Martwy kod

O tym właściwie chciałem napisać w w tym wpisie. Pamiętajmy, ze komentarz jest tak naprawdę częścią kodu. Kodu, o który my - programiści - musimy dbać. Po zmianie algorytmu należy koniecznie zmienić komentarz. Dobry komentarz w dobrym kodzie jest przeważnie bardzo krótki i jest go niewiele. Dobry komentarz w złym kodzie jest przydatny, często niezbędny.

Zły komentarz w każdym kodzie jest niebezpieczny.

Do udowodnienia powyższego zdania posłużę się kawałkiem kodu z książki "Lekcja programowania. (...)".

if ((country == SING) || (country == BRNI) || (country == POL) || (country == ITALY))
{
    /* Jeśli kraj to Singapur, Brunei lub Polska,
       to aktualny czas jest czasem odpowiedzi,
       a nie czasem połączenia.
       Zresetuj czas odpowiedzi i ustaw dzień tygodnia
    */
...

Czego się niby czepiam w tym kodzie? Wszystko śmiga ja ta lala - ładny kod, zmienne mówią nam wszystko, a do tego cały elaborat komentarzy, pozwalający zrozumieć działanie :)... No nie do końca.

Mi nasuwają się dwa pytania:

  1. Co mają wspólnego te państwa ze sobą, że akurat dla nich tak jest?
  2. Co jest nie tak z Włochami, że nie ma ich w komentarzu?

Pierwsze pytanie pokazuje, że ten kod nie jest znowu taki dobry. Autor się postarał -  i rzeczywiście nie jest źle. Jednak stałe z nazwami państw być może mogłyby mówić trochę więcej. A jeśli nie mówią, to może warto by było wytłumaczyć w komentarzu dlaczego właśnie te państwa są wyjątkowe.

Drugie sprawia, że już w ogóle nie wierzę w ten kod, albo nie wierzę w komentarz. Z racji, że komentarz też jest kodem - tyle że martwym - zaczynam mieć drobne uwagi co do całego programu. Spokojnie - widywałem (i sam pisałem:)) gorsze rzeczy. Być może ktoś po prostu zapomniał wspomnieć o Włoszech. Jest też druga możliwość... Aby zachować komentarz aktualnym najpierw usunął informację o Włoszech z komentarza, właśnie miał usunąć także z warunku, kiedy zadzwonił klient z niezwykle-ważną-sprawą i w ten sposób Włochy w kodzie się ostały. Choć nie powinny. Nie wiemy, dlaczego w ogóle ktoś wybrał te państwa.

No dobra - narzekać każdy "umi", a robić nie ma komu. Oto moja propozycja powyższego kodu (pisane w PHP, bo jest mi to bliższy język):

$tradeUnionCountries = array("Singapur", "Poland", "Brunei");
if (in_array($country, $tradeUnionCountries))
{
    $curentTime = getResponseTime();
    setResponseTime(RESET); // albo resetResponseTime();
    setDayOfWeek();
}
else
{
    $curentTime = getConnectionTime();
}

Co to nam dało? Bo przecież mamy więcej kodu:(. O nie, fuj. Nie umiem czytać. Warto tylko pamiętać, że mamy kod "żywy". Jeśli w trakcie jego pisania ktoś nam przerwie, albo sami popełnimy błąd, to zostanie (powinien zostać) on wykryty w trakcie testów. Błędy w komentarzach są niewykrywalne, a po kilku miesiącach trudno określić, co jest nieprawidłowe bez zaprzątania głowy starszym kolegom z firmy (którzy też nie muszą już tego pamiętać). Dodatkowo nazwa zmiennej `$tradeUnionCoutries' mówi nam, że ma to coś wspólnego z jakąś unią handlową. Zawsze można zobaczyć, co za kraje w niej są (dodatkowo jej wartość może być odczytywana bez problemu z zewnętrznego źródła).

A jak Ty napisałbyś taki kod? Wcale nie zakładam, że mój już jest idealny. Być może nawet jest  w nim błąd, którego akurat dzisiaj nie dostrzegam, ale jest bardzo niebezpieczny.

Na koniec nie do końca na temat

Choć ten wpis jest o komentarzach, zmieszczę tu też krótką anegdotkę o nazwach zmiennych. Przy jednym z projektów była sobie zmienna:

$mailDoZdzisia = 'wieslawa.kowalska@nowak.com'

Skąd owy Zdziś? No cóż, kilka lat wcześniej, w firmie pracował Zdziś, do którego miały iść wszystkie wiadomości skądś tam. Później jednak Zdziś zmienił pracę, a owa zmienna została wpisana do światowej listy zabytków zmiennych, co oznacza +/- sytuację,w której każdy już boi się dotknąć fragmentów kodu, które działają. Skąd strach? W starszych projektach niektóre fragmenty działają na słowo honoru. Póki działa nikt nie będzie już tego ulepszał, bo przypomina to malowanie ścian w Titanicu 😉

Morał nawiązujący do tematu: Samokomentujące się nazwy zmiennych i funkcji mogą być niewłaściwe. Nawet w przypadkach kiedy w momencie ich pisania były ekstremalnie poprawne :).

Warto przeczytać

Komentarze (5) Trackbacks (0)
  1. Masz rację 🙂

    Mój błąd. Poprawione. Dzięki za info 🙂

  2. Komentarze w kodzie to podstawa! Szczególnie w takich kolosach jak sklep internetowy. Pracowałem kiedyś po programiście, który w ogóle nie robił komentarzy. Katastrofa jednym słowem!

    • Nie masz racji. Komentarze mogą być przydatne, ale jeśli są niezbędne, to znaczy, że kod jest źle napisany.

      Są kody, które mogą być (muszą) napisane w sposób wymagający komentarzy, np. jakieś bardzo mocno zoptymalizowane obliczenia, zaawansowane algorytmy, czy sztuczki magiczne. Jeśli jednak ktoś uważa, że w takim wypadku musi mieć komentarz:

      if ($n > 78)

      to jest w błędzie! On tu musi _zmienić kod_ na np. taki:

      if ($numberOfLoggedInUsers > MAX_CONNECTIONS)

      czy potrzebujesz tu nadal komentarza?

      A zobacz, że zrobimy tak jakbyś chciał:

      // jesli użytkowników zalogowanych jest więcej niż maksymalna liczba połączeń do bazy danych
      if ($n > 78)

      i się teraz okaże, że:
      1. w trakcie testów wyjdzie, że powinno być `>=’. Programista napisze dodatkowy znak, aby sprawdzić, czy działa. Uruchamia test – działa. Uśmiecha się -> commituje -> niespójność w kodzie i komentarzu zostaje. Za pół roku nikt nie wie, czy to powinno być “>” jak mówi komentarz, czy “>=” jak mówi _działający_ kod. Kto ma rację?

      2. zamiast 78 (czyli liczby określonej np. w konfiguracji bd) ma teraz być 45 (z powodów wydajnościowych jakiś widget jest wyświetlany tylko drobnej liczbie zalogowanych użytkowników). Liczba połączeń do bazy danych nadal wynosi 78. Ale warunek będzie inny. Za rok, pracownik, który to zmienił nie będzie pamiętał dlaczego. Czy zatem komentarz jest słuszny?

      Komentarze nie są złe same w sobie. Są jednak KODEM (choć “martwym”, niewykonywalnym – jedynie “parsowanym przez programistę”), który jeśli w ogóle ulega zmianie to po wszystkich poprawkach. Często jednak w pośpiechu programista zapomni usunąć go, tak jak czasem można znaleźć na SVN (git, czy gdziekolwiek w repozytorium kodu) takie kwiatki:

      // debug(1)

      po prostu ktoś testował i testował, a potem z radości, że wreszcie działa przeoczył jakiś fragment.

      Tak dla komentarzy potrzebnych i prawdziwych. Nie dla komentowania na zapas. YAGNI

  3. Po raz kolejny powtorze – w zlym kodzie komentarz bywa przydatny. Jesli ktos napisal obrzydliwy kod, komentarz bywa niezbedny.

    Jednak zamiast pisac komentarz, moze warto nauczyc sie pisac kod 😉 Samokomentujace sie nazwy zmiennych, czysty kod i odpowiedni zestaw testow pozwalaja na to, aby unikac stosowania komentarzy


Leave a comment

 

Brak trackbacków.