Dokumentowanie kodu źródłowego

3

W mojej dotychczasowej praktyce, komentarze w kodzie wielokrotnie potrafiły mnie wprowadzić w błąd, nie przypominam sobie natomiast przypadku, żeby w czymś pomogły. Podobne zdanie mam o pisaniu javadoca do każdej możliwej metody.
Wyjątki:

  • Zewnętrzne API bibliotek - clean code tu nie wystarczy.
  • hacki, lepiej jak ich nie ma, ale jak są, to dobrze jest mieć komentarz dlaczego jakaś niepotrzebna linia się znalazła w repozytorium
  • metody / klasy o nieoczywistych wymaganiach, stałe, dlaczego jakaś stała to 2.76645, albo że 3 linie obliczeń matematycznych biorą się z jakiegoś twierdzenia/wzoru. Patrząc na sam kod, czasami ciężko się domyślić, że chodzi o twierdzenie Pitagorasa...

Ciekawostka w metodzie, gdzie Sonar zgłosił ostrzeżenie o cognitive complexity ponad miarę:
//NOSONAR If we split this method it would to be harder to read

3

Trochę bez sensu pisać komentarze do każdej linijki, wtedy w niczym nie pomogą. W takich przypadkach wywalam regexpem komentarze z całego pliku i staram się dojść do logiki na podstawie samego kodu. Otwieram wersję oryginalną obok i zaglądam tylko aby upewnić się, że komentarz nie dotyczy czegoś ważnego.

Słabe jest też duplikowanie logiki w komentarzu, bo to kod ma opisywać logikę, pisać drugi raz komentarz jest raczej bez sensu, bo wtedy przy zmianie logiki trzeba pamiętać o zmianie obu rzeczy. Pisanie komentarza jako notatkę dla siebie za 6 miesięcy też raczej warto sobie darować.

Opisywanie podejścia, albo historii podejść, albo dlaczego tak a nie inaczej, dla mnie ma duży sens. Bo jeśli ktoś spędził tydzień na researchowaniu problemu i napisał jakąś implementację, a potem przychodzi junior i chce to refactorować bo wie lepiej, to wtedy taki napisany komentarz może juniorowi oszczędzić trochę czasu ;).

0

Ok, z tą każdą linią może trochę przesadziłem.

Wklejam przykładowy kod PHP, jedna z metod:

  /**
   * Sprawdza dzisiejsze premiery i powiadamia użytkowników, musi być uruchomione w trybie CLI.
   */
  public static function checkTodayPremieres() {

    // Sprawdzenie trybu CLI, brak trybu CLI = koniec
    if ( !\inopx\http\CLIChecker::isCLI() ) {
      return;
    }

    // PDO
    $PDO = \inopx\db\ConnManager::getDefaultConnection()->getInternalHandler();

    // Sprawdzamy dzisiejsze premiery
    $stmt = $PDO->prepare("SELECT * FROM k3_product WHERE date_premiere IS NOT NULL AND DATE(date_premiere) = CURRENT_DATE");
    $stmt->execute();
    $allProducts = $stmt->fetchAll( \PDO::FETCH_ASSOC );

    // Brak premier - koniec
    if (!$allProducts || empty($allProducts)) {
      return;
    }

    // Iteracja po dzisiejszych premierach i powiadamianie klientów
    foreach ($allProducts as &$product) {

      $p = new \k3\product\K3ProductEntity( $product );
      $p->afterFetch();
      self::productHasPremiere($p);

    }

  }

Trochę retorycznie: chyba wygodniej czyta się coś takiego, niż napaćkane ciurkiem bez żadnego komentarza?

3

Mnie uczono, że kod ma być samo opisujący.
Komentarze dodaję w ostateczności.

8

@mlody_d i tak jest, co do co robi kod. Komentarze są ok do opisania czemu albo jak. Przykład wyżej od @TomRZ to idealny przykład jak nie pisać komentarzy. np.:

    // Brak premier - koniec
    if (!$allProducts || empty($allProducts)) {
      return;
    }

To dramat. Wystarczyło zrobić metodę czyBrakPremier() i nie trzeba by pisać komentarza. Podobnie:

    // Sprawdzamy dzisiejsze premiery
    $stmt = $PDO->prepare("SELECT * FROM k3_product WHERE date_premiere IS NOT NULL AND DATE(date_premiere) = CURRENT_DATE");
    $stmt->execute();
    $allProducts = $stmt->fetchAll( \PDO::FETCH_ASSOC );

Czemu nie zrobić metody fetchTodayPremieres()? o_o
Analogicznie to całe // Iteracja po dzisiejszych premierach i powiadamianie klientów, czemu nie ma metody notifyClientsAboutTodaysPremiers()?

0

Czemu nie zrobić metody fetchTodayPremieres()? o_o
Analogicznie to całe // Iteracja po dzisiejszych premierach i powiadamianie klientów, czemu nie ma metody notifyClientsAboutTodaysPremiers()?

Oczywiście róbmy osobną metodę dla 3 linijek kodu, i używajmy tą metodę tylko w jednym miejscu. Bardzo rozsądne. LOL

5

@TomRZ:

Oczywiście róbmy osobną metodę dla 3 linijek kodu, i używajmy tą metodę tylko w jednym miejscu. Bardzo rozsądne. LOL

A dlaczego nie? O "S" z SOLID słyszałeś?

A już mniej konfrontacyjnie:


  public static function checkTodayPremieresAndNotifyUsers() {

      $premieres = checkTodayPremieres();

      notifyAllUsers($premires)
    }

  }

Nie pisałem w PHP 20 lat, pewnie coś się zmieniło, ale chodzi o oddanie idei. Czy potrzebujesz jakiegokolwiek komentarza do takiej metody?

7

@TomRZ ja rozumiem że ciężko przyjąć krytykę, ale co to pokazałeś jest słabe i tyle. Możesz zamyknąc oczy i twierdzić ze tak nie jest, ale powie ci to w zasadzie każdy programista. Gdybyś kiedykolwiek miał code review albo zespól to być o tym wiedział. Powie ci to dowolna książka z serii Clean Code. Powie ci to Sonar i dowolne narzędzie do statycznej analizy kodu. Nie wierzysz? Sprawdź!
Możesz zrobić ankietę na forum z tym kodem i niech każdy oceni co o tym sądzi :)

I nie, komentarze nie są tu największym problemem. Problem to łamanie SOLIDa, mieszkanie poziomów abstrakcji (jakieś operacje biznesowe obok gołych stringów SQLowych o_O), nazwa metody checkTodayPremieres ktora w rzeczywistości zajmuje się wysyałniem notyfikacji... komentarze po polsku to tylko wisienka na torcie.

edit: żeby było jasne, to nie jest nic osobistego. Nikt nie pisze idealnego kodu i każdy z nas ma pewnie kupę komentarzy od reviewerów pod każdy pull requestem. To jest zupełnie normalne i dzięki temu człowiek się rozwija i z czasem pisze lepiej.

2

Kod samodokumentujacy to podstwa. Z drugiej strony nie wyobrażam sobie braku komentarzy dla rzeczy nieoczywistych. Przykładem dla mnie jest chociażby hak matematyczny użyty w quake do liczenia paroboli chyba. Zamiast coś liczyć matematycznie co obciążało CPU to wprowadzili jakiś współczynnik, który to robił z bardzo dużą dokładnością. Ponieważ nawet teraz nie jestem w stanie sobie przypomnieć co i jak to było to tym bardziej ani nie zrozumiałbym wagi tego haka, ani nie pamiętałbym jak działa i dlaczego.

2
TomRZ napisał(a):

Czemu nie zrobić metody fetchTodayPremieres()? o_o

Analogicznie to całe // Iteracja po dzisiejszych premierach i powiadamianie klientów, czemu nie ma metody notifyClientsAboutTodaysPremiers()?

Oczywiście róbmy osobną metodę dla 3 linijek kodu, i używajmy tą metodę tylko w jednym miejscu. Bardzo rozsądne. LOL

:D metody się tworzy głównie dla czytelności a nie z przymusu żeby móc użyć kodu w dwóch miejscach. Co więcej - im mniej użyć metod tym lepiej (poza utilsami)
Zaskoczę Cię też że np uncle bob sugeruje żeby żadna funkcja / metoda NIGDY nie była dłuższa niż 3 linijki i tak - jest to możliwe.

W Twoim kodzie mimo komentarzy nie mam pojęcia co się dzieje:

    // Iteracja po dzisiejszych premierach i powiadamianie klientów
    foreach ($allProducts as &$product) {

      $p = new \k3\product\K3ProductEntity( $product );
      $p->afterFetch();
      self::productHasPremiere($p);

która z tych trzech linijek niby "powiadamia klientów"?

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0