Czy przegląd kodu, który wykorzystuje tylko komentarze do kodu, jest dobrym pomysłem?

16

Warunki wstępne

  • Zespół używa DVCS
  • IDE obsługuje parsowanie komentarzy (np. TODO itp.)
  • Narzędzia takie jak CodeCollaborator są drogie ze względu na budżet
  • Narzędzia takie jak gerrit są zbyt skomplikowane do zainstalowania lub nie nadają się do użytku

Przepływ pracy

  • Autor publikuje gdzieś w centralnej gałęzi funkcji repo
  • Recenzent pobierz go i zacznij recenzję
  • W przypadku niektórych recenzentów pytań / problemów utwórz komentarz ze specjalną etykietą, np. „REV”. Taka etykieta MUSI nie znajdować się w kodzie produkcyjnym - tylko na etapie recenzji:

    $somevar = 123;
    // REV Why do echo this here?
    echo $somevar;
    
  • Kiedy recenzent kończy dodawanie komentarzy - po prostu popełnia głupią wiadomość „komentarze” i odpycha

  • Autor wycofuje gałąź funkcji i odpowiada na komentarze w podobny sposób lub poprawia kod i wypycha go z powrotem
  • Kiedy komentarze „REV” znikną, możemy pomyśleć, że przegląd został pomyślnie zakończony.
  • Autor interaktywnie zmienia gałąź funkcji, zgniata ją, aby usunąć zatwierdzenia „komentarza”, a teraz jest gotowy do scalenia funkcji w celu opracowania lub wykonania dowolnej czynności, która zwykle mogłaby być przeprowadzona po udanej wewnętrznej ocenie

Obsługa IDE

Wiem, że niestandardowe tagi komentarzy są możliwe w eclipse i netbeans. Jasne, że powinno to również należeć do rodziny blablaStorm.

Przykład niestandardowego filtrowanego zadania z komentarzy w środowisku Eclipse

pytania

  1. Czy uważasz, że ta metodologia jest wykonalna?
  2. Czy znasz coś podobnego?
  3. Co można w nim poprawić?
gaRex
źródło
Dobre pytanie, ale nie ma to nic wspólnego z serwetkami - niezbyt dobry tytuł.
MarkJ
@MarkJ jak to nazwać? Mam na myśli coś rękodzielniczego, wiejskiego, domowego. W języku rosyjskim mamy frazę „на коленке”. Coś niestabilnego, nie idealnego, niestałego, ale to działa.
gaRex,
2
@MarkJ, gaRex: co z tym nowym tytułem? Cofnij, jeśli uznasz, że to nie jest właściwe dla tego pytania.
Arseni Mourzenko
@MainMa, w porządku
gaRex,
1
Atlassian Crucible jest zasadniczo darmowy dla maksymalnie 10 programistów. Zdarza się również, że jest to najlepsze narzędzie do sprawdzania kodu, z jakiego korzystałem. Podejście do komentowania jest wykonalne, ale może utrudnić śledzenie stanu.
Andrew T Finnell,

Odpowiedzi:

14

Pomysł jest bardzo fajny. W przeciwieństwie do typowych przepływów pracy, przeglądasz bezpośrednio w kodzie, więc technicznie nie potrzebujesz niczego oprócz edytora tekstu, aby korzystać z tego przepływu pracy. Wsparcie w IDE jest również miłe, szczególnie możliwość wyświetlania listy recenzji na dole.

Nadal istnieje kilka wad:

  • Działa dobrze w przypadku bardzo małych zespołów, ale większe zespoły będą wymagać śledzenia tego, co zostało sprawdzone, kiedy, przez kogo iz jakim rezultatem. Chociaż faktycznie masz tego rodzaju śledzenie (kontrola wersji pozwala o tym wszystkim wiedzieć), niezwykle trudno jest go używać i wyszukiwać, i często wymaga ręcznego lub półautomatycznego przeszukiwania wersji.

  • Nie wierzę, że recenzent ma wystarczającą ilość opinii od recenzenta, aby wiedzieć, jak faktycznie sprawdzono punkty .

    Wyobraź sobie następującą sytuację. Alice po raz pierwszy sprawdza kod Erica. Zauważa, że ​​Eric, młody programista, zastosował składnię, która nie jest najbardziej opisowa w faktycznie używanym języku programowania.

    Alice sugeruje alternatywną składnię i przesyła kod z powrotem do Erica. Przepisuje kod przy użyciu tej alternatywnej składni, która jego zdaniem jest zrozumiała poprawnie, i usuwa odpowiedni // BLAkomentarz.

    W następnym tygodniu otrzyma kod do drugiej recenzji. Czy byłaby w stanie pamiętać, że wypowiedziała tę uwagę podczas swojej pierwszej recenzji, aby zobaczyć, jak Eric ją rozwiązał?

    W bardziej formalnym procesie recenzji Alice natychmiast zauważyła, że ​​zrobiła uwagę, i zobaczyła różnicę w odpowiednim kodzie, aby zauważyć, że Eric źle zrozumiał składnię, o której mu powiedziała.

  • Ludzie wciąż są ludźmi. Jestem prawie pewien, że niektóre z tych komentarzy znajdą się w kodzie produkcyjnym, a niektóre pozostaną śmieciami, będąc całkowicie nieaktualnymi .

    Oczywiście ten sam problem istnieje w przypadku każdego innego komentarza; na przykład w produkcji jest wiele komentarzy TODO (w tym najbardziej przydatny: „TODO: Skomentuj poniższy kod.”) i wiele komentarzy, które nie zostały zaktualizowane, gdy odpowiedni kod był.

    Na przykład oryginalny autor kodu lub recenzent może odejść, a nowy programista nie zrozumie, co mówi opinia, więc komentarz pozostanie na zawsze, oczekując, że ktoś będzie zbyt odważny, aby go usunąć lub faktycznie zrozumieć to mówi.

  • To nie zastępuje recenzję twarzą w twarz (ale ten sam problem dotyczy również innych bardziej formalnego przeglądu, który nie odbywa się twarzą w twarz).

    Zwłaszcza jeśli oryginalna recenzja wymaga wyjaśnień, recenzent i recenzent rozpoczną rodzaj dyskusji . Nie tylko znajdziesz duże komentarze BLA, ale te dyskusje również zanieczyszczą dziennik kontroli wersji .

  • Możesz również napotkać drobne problemy ze składnią (która istnieje również w przypadku komentarzy TODO). Na przykład, jeśli długi komentarz „// BLA” pojawi się w kilku wierszach, a ktoś zdecyduje się napisać go w ten sposób:

    // BLA This is a very long comment which is way beyond 80 characters, so it actually
    // fills more then one line. Since the next lines start with slashes, but not "BLA"
    // keyword, the IDE may not be able to show those lines, and display only the first one.
    
  • I wreszcie jako drobna i bardzo osobista uwaga: nie wybieraj słowa „BLA” jako słowa kluczowego. Brzmi brzydko. ;)

Arseni Mourzenko
źródło
„aby dowiedzieć się, w jaki sposób zastosowano sprawdzone punkty” Tak :) Tylko uczciwość recenzenta. Tutaj mamy więcej zasad niż narzędzi.
gaRex
1
„ludzie to ludzie” Tak :( Mamy już miliony takich TODO. Czy można po prostu odmówić posiadania takiej funkcji w IDE?
gaRex
„zanieczyszcz dziennik kontroli wersji”. W tym celu cała praca znajduje się w niezależnej gałęzi funkcji, która później jest zgniatana i łączona w fazie rozwoju. Może to być zrobione przez prosty hak w DVCS. Ale jak widzę, cała złożoność przenosi się z narzędzi do sprawdzania kodu do IDE i DVCS.
gaRex,
„drobne problemy ze składnią” Może to nie jest problem? Te REV są tylko niektórymi markerami, aby szybko do nich przejść z panelu.
gaRex
1
@gaRex: wtedy członkowie twojego zespołu powinni użyć adresu e-mail, aby uzgodnić datę spotkania twarzą w twarz ;-)
Doc Brown
4

Uzupełnię komentarze w kodzie o dokument towarzyszący. Podsumowałoby to ustalenia i trwałoby po usunięciu komentarzy. Zaletami tego byłyby:

  • ścisłość. Jeśli osoba rutynowo nie sprawdza, czy przekazany wskaźnik nie ma wartości zerowej (lub innego typowego błędu początkującego w używanym języku), recenzent może zostawić dziesiątki komentarzy REV w tym celu, aw dokumencie może powiedzieć „ Znalazłem 37 miejsc, w których wskaźniki nie były sprawdzane jako pierwsze ”, nie wymieniając ich wszystkich
  • miejsce pochwały. Komentarz REV this is a nice designwydaje się po prostu dziwny, ale moje recenzje kodu często zawierają zarówno zatwierdzenie, jak i poprawki
  • interaktywność. Twój przykładowy komentarz brzmi „dlaczego to zrobiłeś?” i jest to bardzo typowy. Podejście tylko do komentowania nie obsługuje rozmowy. Osoba może zmienić kod i usunąć komentarz lub usunąć komentarz. Ale „dlaczego to zrobiłeś?” a „proszę to zmienić, to źle” to nie to samo.
  • prowadzenie rejestru. Późniejszy recenzent może sprawdzić, czy kod rzeczywiście został zmieniony, czy też komentarze zostały właśnie usunięte. Mogą również sprawdzić, czy komentarze zostały „uwzględnione”, a programista nie popełnia już tych samych błędów w kolejnej recenzji.

Chciałbym również użyć elementu pracy do wykonania recenzji i udzielenia odpowiedzi na recenzję oraz powiązać z nią kontrole. Ułatwia to znajdowanie komentarzy w starym zestawie zmian i sprawdzenie, jak każdy z nich był obsługiwany w innym zestawie zmian.

Kate Gregory
źródło
potrzebujemy dobrego narzędzia do sprawdzania kodu. Nasze obecne opisane podejście jest głównie polityczne, ponieważ ludzie powinni wynaleźć jakąś etykietę i postępować zgodnie z nią.
gaRex,
„zwartość” - myślę, że można to zrobić za pomocą jednego komentarza, takiego jak „// REV Koleś, mamy 37 miejsc, w których wskaźniki nie były najpierw sprawdzane, w tym ten. Proszę RTFM”
gaRex
„miejsce na pochwałę” - również możliwe, ale po zgnieceniu zostanie utracone :( Widzę już jeden problem - straciliśmy historię recenzji przy zgniataniu.
gaRex
„interaktywność” - autor może odpowiedzieć w innym komentarzu, poniżej wstępnego. Podobnie jak w stylu wikipedia.
gaRex,
„osoba może ... usunąć komentarz” - to problem. +1
gaRex
2

Inni mówili o ograniczeniach tego podejścia. Wspomniałeś, że narzędzia takie jak gerrit były trudne do zainstalowania - proponuję spojrzeć na phabricator (http://www.phabricator.com). Jest to system sprawdzania kodu, z którego Facebook korzysta od lat, a ostatnio został otwarty. Nie jest trudny do zainstalowania, ma doskonałe przepływy pracy i rozwiązuje wszystkie problemy wymienione w innych komentarzach.

Adam Hupp
źródło
łał! Musimy wypróbować to zamiast naszej uroczej czerwonawy.
gaRex
„gerrit” zainstalowałem go - nie było tak trudne. Po prostu tego nie używam! A także ma brzydki interfejs użytkownika i przepływ pracy.
gaRex
@gaRex Używamy Reviewboard ( reviewboard.org ) równolegle z Redmine. Służą one różnym celom, więc wszystko jest w porządku. I możesz skonfigurować RB, aby łączył się z problemami Redmine ...
Johannes S.,
@JohannesS. z jakich vcs korzystasz? Czy masz też gotowe instrukcje lub wiki na temat ich integracji? Miło mieć taki.
gaRex
@gaRex Przepraszamy za opóźnienie w odpowiedzi. Używamy SVN, ale RB obsługuje również git (tak naprawdę ludzie RB sami używają git). Sugeruję zajrzeć na stronę RB. Dostępne jest demo online (np. Sprawdź demo.reviewboard.org/r/101 )
Johannes S.,