Jakie są typowe procesy sprawdzania kodu i co jest uważane za złe?

Moja firma niedawno zaczęła przeprowadzać sformalizowane przeglądy kodu. Proces przebiega w następujący sposób: prześlesz do github, poprosisz o żądanie ściągnięcia, kod zostanie sprawdzony przez około trzy osoby, a jeśli wszystko przejdzie, twój kod wejdzie.

Proces wydaje się być sprawiedliwy, jednak trzy osoby, które dokonują recenzji kodu, nie wydają się uczciwe. Zauważam, że kiedy poddaję kod do sprawdzenia, dostaję od 100 do 200 komentarzy. Najwyższy numer dla mnie to 300 komentarzy raz. Oczywiście można by pomyśleć, że to duże zmiany, ale mogą to być również bardzo małe zmiany z mniej niż 50 liniami kodu (w tym testy jednostkowe). Wszystkie komentarze są uważane za „konieczne” i bez argumentów.

Mając to na uwadze, moim głównym problemem tutaj jest to, że wydaje się nieco przesadne. Rozmawiałem z grupą i powiedzieli mi w zasadzie, że to, że miałem tyle lat rozwoju w php, nie oznacza, że ​​jestem „programistą”. Oczywiście wydaje się to bardziej bolesne niż nie. Zauważam również, że w grupie nie wydają się tak dużo komentarzy i przez większość czasu ignorują lub w inny sposób ignorują inne komentarze lub sugestie, rzadko akceptują to jako ważny punkt, nawet jeśli coś jest zepsute.

Więc moje pytanie brzmi, czy to jest sprawiedliwe? Czy często?

Wszystkie komentarze są uważane za „konieczne” i bez argumentów.

IMHO to prawdziwy problem, ponieważ nie ma w tym priorytetów. Gdy dojdziesz 100-300 komentarze, nie muszą być niektóre z nich ma pierwszeństwo A (Real bugów), niektóre z nich prio B (może prowadzić do błędów późniejszych), a niektóre z nich prio C (wszystko). Powiedz kolegom, że chcesz uszanować wszystkie ich życzenia, ale wprowadzić zmiany w życie, a twój czas jest ograniczony, nalegaj na ustalenie priorytetów. Następnie zacznij od ustalenia komentarza na temat prio A, a jeśli naprawdę będziesz miał na to więcej czasu, możesz zacząć od B (jeśli masz szczęście, twój szef zrozumie, że ustalenie prio B i C nie jest tak ważne i da ci kilka ważniejszych zadań zamiast marnowania czasu).

Przeglądy kodu mogą być procesem dzielącym.

Jesteś jednak na ważnym skrzyżowaniu. Dokonaj przemyślanej analizy ich recenzji. Czy identyfikują problemy związane z wybieraniem nitów, czy podkreślają poważne wady stylu i logiki?

Jeśli jest to pierwsze, zalecam pracę nad rozwiązaniem (nowe zadanie lub nowe procesy sprawdzania kodu).

Jeśli to drugie, zalecam dużo czytania i studiowania kodu, aby spróbować poprawić swój kod do profesjonalnej jakości.

Z twoich komentarzy wynika, że ​​twoi koledzy używają procesu przeglądu kodu do uzgodnienia metodologii lub dopracowania kodu. Właśnie zacząłem robić recenzje kodu, takie jak Ty, i zauważam, że czasami dyskutujemy dużo o rzeczach, które są tylko podejściami do implementacji lub ulepszeniami. Nie jest to wcale takie złe, o ile jest to uzasadnione (300 komentarzy wygląda dla mnie zbyt wiele, co musi wyglądać jak wątek reddit)

Być może musisz uzgodnić pewne decyzje architektoniczne dotyczące kodu, zanim zaczniesz go implementować, a może po prostu zgadzasz się co do konwencji nazewnictwa, wzorców i dobrych praktyk, aby wszyscy wiedzieli, co to jest uważane za „dobry kod”.

Jeśli przestrzegasz standardów kodu, jak mówisz, a kod działa zgodnie z przeznaczeniem, nie powinno być tak wielu komentarzy, więc albo używają Twojego kodu jako forum, albo trollują cię, jak się wydaje, że wskazujesz.

Starałbym się krytykować siebie, brać udział w rozmowach i widzieć przyczynę wszystkich tych komentarzy, a może rozmawiać z nimi w konstruktywny sposób, aby zobaczyć, dlaczego są tak niezadowoleni z twojego kodu i jeśli możesz kodu w sposób, który sprawia, że ​​wszyscy są szczęśliwi, a praca nie utknie w przeglądzie kodu.

Właśnie czytam twoje ostatnie komentarze, czasami, gdy nie zgadzasz się z kodem, możesz go przejrzeć sto razy i zaproponować zmiany wszędzie, które Cię nie uszczęśliwią, bo prawdziwy powód, dla którego podjąłbyś inną decyzję architektoniczną i po prostu nie podoba ci się ten kod, bez względu na to, ile razy go refaktoryzujesz. Jak mówię powyżej, być może musisz wcześniej uzgodnić podejście do kodu, więc pisząc go, wiesz, czego się od niego oczekuje, a zatem twój kod byłby dla nich bardziej rozsądny.

Z tego, co mówisz, wydaje mi się, że mogą mieć uprzedzenia wobec programistów php, a zatem próbują znaleźć każdą rzecz, która jest zła w twoim kodzie, aby udowodnić ich rację .¹

Jeśli chodzi o samą recenzję kodu, uważam, jak już powiedziałeś, że tak ogromna liczba drobnych komentarzy jest mniej pomocna niż kilka dobrych i uzasadnionych uwag. I chociaż mam ograniczone doświadczenie w zakresie recenzji kodu, następująca technika sprawdziła się w zespołach, w których pracowałam w przeszłości.

• Przede wszystkim należy użyć statycznego analizatora kodu, aby zidentyfikować większość problemów przed przeglądem kodu. Np .: uruchomienie kodu za pomocą Sonaru, Linta lub innego dobrego analizatora kodu powinno pomóc ci pozbyć się większości drobnych problemów. Zwłaszcza, że ​​twoi recenzenci mogą definiować profile niestandardowe, aby zapewnić wszystko, od umieszczania nawiasów, białych znaków, komentarzy, prawidłowego nazewnictwa zmiennych i wiele więcej ...
• Po drugie, wydaje mi się, że dobrze działa, jeśli podzielisz komentarze na różne kategorie. Na przykład dwie kategorie, w których jedna grupa zawiera małe rzeczy, które należy wziąć pod uwagę i zastosować w przyszłości. I druga grupa dla tych komentarzy, które wymagają natychmiastowej modyfikacji kodu, która wymagałaby kolejnego zatwierdzenia i późniejszej recenzji. Oczywiście liczba komentarzy w drugiej grupie powinna być mniejsza.

Co więcej, muszę powiedzieć, że moje pierwsze prawdziwe recenzje kodu zawierały także więcej komentarzy, niż początkowo się spodziewałem. Jednak nigdy nie uważałem tego za coś złego. Jeśli nadal uczysz się na podstawie ich komentarzy² i chcesz zastosować te nowo wyuczone techniki / najlepsze praktyki w swoich przyszłych kodach, komentarze powinny być mniejsze. Z pewnością tak było w moim przypadku ;-)

¹ Z mojego doświadczenia wynika, że ​​zdarza się to często, ponieważ wielu programistów twierdzi, że php jest najbardziej złym językiem programowania, z którego korzystają najbardziej niedoświadczeni programiści. Oddalam się od tego stwierdzenia, ponieważ wierzę, że świetne oprogramowanie można napisać w dowolnym języku!

² Zakładając, że choć komentarze są przesadzone, zawiera się w nich pewna wartość

Czy ktoś rutynowo otrzymuje ponad 100 komentarzy w recenzjach kodu? Powiedziałbym, że nie. Czy zdarza się, że ludzie, których jakość kodu „pozostawia wiele do życzenia”, otrzymują absolutnie dużo komentarzy.

Zależy to jednak również od „reguł” procesu przeglądu kodu. KAŻDY ma własne pomysły na to, jak coś należy zrobić. Jeśli proces sprawdzania kodu pozwala na komentarze w postaci „Powinieneś to zrobić w ten sposób zamiast w ten sposób”, prawdopodobnie dostaniesz DUŻO komentarzy nawet dla odpowiedniego kodu. Jeśli proces ma na celu wykrycie „wad”, liczba komentarzy powinna być znacznie mniejsza.

Z mojego doświadczenia wynika, że ​​recenzje, które pozwalają na „sugestie” dotyczące alternatywnych metod, są marnowaniem czasu. Te „sugestie” powinny być obsługiwane osobno poza procesem przeglądu. Recenzje defektów są bardziej przydatne, ponieważ pozwalają ludziom skupić się na błędach zamiast „dlaczego nie zrobiłeś tego tak, jakbym to zrobił?”. Jest to również bardziej przydatne, ponieważ nie można odmówić błędu, jeśli ktoś go znajdzie. Zatem nie ma zranionych uczuć, ale raczej wdzięczność.

AKTUALIZACJA: Mimo wszystko, niektóre kody są po prostu złe, nawet jeśli nie zawierają wad. W takim przypadku komentarz do recenzji powinien być pojedynczym komentarzem, który mówi coś w rodzaju. „Ten kod musi zostać wyczyszczony. Odłóż recenzję do czasu omówienia kodu z [twoje imię tutaj].” W takim przypadku dalszy przegląd kodu powinien zostać zatrzymany do momentu skorygowania komentarza.

AKTUALIZACJA2: @ Użytkownik: Czy omawiasz swój kod / projekt z jednym z nich podczas jego opracowywania, abyś mógł zaimplementować to, czego szukają, zanim zajdziesz daleko na swój sposób? Zmieniasz coś w tym, jak rozwijasz kod na podstawie ich sugestii, czy nadal myślisz, że po swojemu jest dobrze? Czy uczysz się czegoś na podstawie ich komentarzy?

Kiedy jestem liderem projektu, odpowiadam za WSZYSTKIE produkty pracy. Jeśli zatwierdzę produkt roboczy, twierdzę, że produkt jest akceptowalny. Chcę mieć reputację w budowaniu produktów wysokiej jakości. Tak więc mam oczekiwania i nie zaakceptuję mniej niż zadowalające. Jednocześnie staram się uczyć i wyjaśniać powody moich preferencji. Te preferencje nie zawsze są idealne (szczególnie w oczach innych), ale większość z nich pochodzi z doświadczenia. Zwykle reakcja, aby uniknąć powtórzenia złych. W związku z tym istnieje kilka moich osobistych „sticklerów”, które są niezbędne, aby uzyskać moją zgodę, niezależnie od odpowiedzi.

Z drugiej strony musisz poznać oczekiwania niezbędne do zatwierdzenia produktów do pracy. Możesz się nie zgodzić, ale ponieważ wydaje się, że nie masz uprawnień do nadrzędnego rządzenia, dowiedz się, czego się spodziewać. Wątpię, aby zespół próbował sprawić, byś poniósł klęskę. To sprawia, że ​​również źle wyglądają. W związku z tym po prostu pokaż, że chcesz się uczyć (nawet jeśli nie jesteś), weź to, co mówią, i postaraj się dostosować do ich preferencji, a zapewne ich nie wycofasz. Może znajdź ten, który możesz przynajmniej tolerować i sprawdź, czy zrobią to z ręką na rękę, by nauczyć cię swoich sposobów. Kto wie, w trakcie tego procesu możesz nauczyć się czegoś, co naprawdę może przenieść twoje umiejętności na wyższy poziom.

Kilka ważnych różnic w procesie inspekcji naszego zespołu:

• podstawą inspekcji jest lista kontrolna opracowana przez cały zespół.
• Koncentracja to wady (teraźniejszość i przyszłość), a nie styl ze względu na styl.
• Trzej inspektorzy (w tym autor) siedzą razem, aby przejrzeć komentarze. Pozostają tylko komentarze z większością głosów.

Dla 50 LOC 300 komentarzy wydaje się nieco przesadzonych i - wow - 3 recenzentów na każde żądanie ściągnięcia? Twoja firma musi mieć dużo zasobów.

Z mojego doświadczenia w zakresie przydatnego procesu sprawdzania kodu muszą istnieć pewne zasady i / lub wytyczne:

• Priorytet komentarzy
• Klasyfikacja komentarzy (błąd, styl kodu itp.)
• Uzgodniona architektura / projekt do naśladowania
• Uzgodniony styl kodu

Jeśli recenzenci nie otrzymają pierwszeństwa, poproś odpowiedzialnego kierownika projektu / kierownika zespołu; osoba odpowiedzialna za koszty powinna mieć opinię na temat priorytetów.

Jeśli masz zdefiniowaną architekturę, powszechne zrozumienie, jakich wzorców projektowych używasz w projekcie i uzgodnionego stylu kodu, komentarze w recenzjach powinny dotyczyć tylko „rzeczywistych problemów”, takich jak kwestie bezpieczeństwa, niezamierzone błędy, przypadki narożne nie ujęte w określonym architektura itp.

Jeśli Twój zespół programistów nie zgodził się na „problemy smakowe” (np. Czy zmienna członkowska powinna zaczynać się od „m_”), to każdy recenzent zmusi cię do przestrzegania jego stylu, co jest po prostu stratą czasu / pieniędzy.

To naprawdę brzmi jak problem z komunikacją. Oczekujesz, że Twój kod nie jest wystarczająco zły, aby zasłużyć na 300 komentarzy. Recenzenci wydają się sądzić, że potrzebujesz dużo informacji zwrotnych. Asynchroniczne kłótnie w przód i w tył marnują dużo czasu. Do licha, pisanie 300 komentarzy to PRAWDZIWA strata czasu. Jeśli to nie wszystkie wady, to możliwe, że jako nowy członek zespołu nie znasz jeszcze stylu zespołu. To normalne i należy się czegoś nauczyć, aby przyspieszyć cały zespół.

Moja sugestia to oszczędność czasu. Przyspiesz sprzężenie zwrotne. Ja bym:

• Wykonuj więcej okresowych recenzji, aby uniknąć popełnienia tego samego błędu i wygenerowania tego samego komentarza 50 razy
• Usiądź z recenzentem, który przegląda Twój kod, abyś mógł porozmawiać o pojawiających się problemach, unikając w ten sposób udokumentowania 300 „problemów”, które zostaną wymazane przy następnym zatwierdzeniu
• Połącz się z jednym z tych „prawdziwych” programistów przez jakiś czas, pisząc kod, aby zobaczyć, co zrobiliby inaczej

Ludzie mogą się kłócić przeciwko parowaniu, ponieważ „potrwa to dłużej”, ale oczywiście nie jest to problemem.