Frustracje związane z recenzowaniem / kodowaniem

27

Nie nazwałbym siebie supergwiazdą, ale stosunkowo doświadczonym. Staram się utrzymywać jakość kodu na wysokim poziomie i zawsze staram się ulepszyć mój styl kodowania, staram się, aby kod był wydajny, czytelny i spójny, a także zachęcać zespół do przestrzegania wzorców i metodologii w celu zapewnienia spójności. Rozumiem także potrzebę równowagi między jakością i szybkością.

Aby to osiągnąć, przedstawiłem zespołowi koncepcję wzajemnej oceny. Dwa kciuki do góry w github-prośbie o połączenie. Świetnie - ale moim zdaniem nie bez czkawek.

Często widzę komentarze tych samych kolegów, takie jak -

  • Dobrze byłoby dodać spację później <INSERT SOMETHING HERE>
  • Niechciana dodatkowa linia między metodami
  • Na końcu komentarzy w docblocks należy stosować kropkę.

Teraz z mojej perspektywy - recenzent powierzchownie patrzy na estetykę kodu - i tak naprawdę nie dokonuje przeglądu kodu. Przegląd kodu kosmetycznego przychodzi mi do głowy jako arogancka / elitarna mentalność. Brakuje mu treści, ale tak naprawdę nie można się z tym zbytnio kłócić, ponieważ recenzent jest technicznie poprawny . Wolałbym zobaczyć mniej z powyższych rodzajów recenzji, a więcej recenzji w następujący sposób:

  • Możesz zmniejszyć złożoność cykliczną, ...
  • Wyjdź wcześniej i unikaj if / else
  • Wyodrębnij zapytanie DB do repozytorium
  • Ta logika tak naprawdę tu nie pasuje
  • Nie powtarzaj się - streszczenie i powtórne użycie
  • Co by się stało, gdyby Xzostał przekazany jako argument do metody Y?
  • Gdzie jest na to test jednostkowy?

Uważam, że zawsze są to ci sami ludzie, którzy oceniają kosmetyczne typy recenzji, i ci sami ludzie, którzy moim zdaniem wydają recenzje oparte na jakości i logice.

Jakie (jeśli w ogóle) jest prawidłowe podejście do wzajemnej oceny. I czy mam rację, gdy frustrują mnie te same osoby, które w zasadzie przeglądają kod, szukając błędów ortograficznych i defektów estetycznych zamiast faktycznych defektów kodu?

Jeśli mam rację - jak zachęcić kolegów do szukania błędów w kodzie w równowadze z sugerowaniem kosmetycznych poprawek?

Jeśli się mylę - proszę, oświeć mnie. Czy istnieją jakieś praktyczne zasady dotyczące tego, co stanowi dobrą recenzję kodu? Czy przegapiłem punkt, czym są recenzje kodu?

Z mojego punktu widzenia - przegląd kodu dotyczy wspólnej odpowiedzialności za kod. Nie czułbym się dobrze, podchodząc do kodu bez adresowania / sprawdzania logiki, czytelności i funkcjonalności. Nie zawracałbym sobie również głowy blokowaniem scalania solidnego fragmentu kodu, gdybym zauważył, że ktoś pominął kropkę w bloku doc.

Kiedy sprawdzam kod, spędzam może od 15 do 45 minut na 500 loc. Nie wyobrażam sobie, by te płytkie recenzje trwały dłużej niż 10 minut, jeśli taka jest głębokość recenzji, którą wykonują. Ponadto, jaką wartość ma kciuk od płytkiego recenzenta? Z pewnością oznacza to, że wszystkie kciuki nie są równej wadze i może być konieczny 2-etapowy proces przeglądu. Jeden kciuk do głębokich recenzji i drugi kciuk do „polerowania”?

Sos
źródło
2
Czy próbowałeś o tym wspomnieć tej osobie?
Bryan Oakley,
11
Miałem przełożonego, który wymagał kropek we wszystkich komentarzach, a także prawidłowej pisowni wielkimi literami, interpunkcji i gramatyki. Był także bardzo analny w kwestii białych znaków. Doprowadziło mnie to do szaleństwa, ale doprowadziło również do bardzo czytelnego, spójnego kodu.
Bryan Oakley,
4
Pierwsze trzy kule w twoim poście to zrzucanie rowerów, chyba że są one częścią standardowego sklepu w stylu kodowania. Jeśli piszesz dokumentację, oczekiwałbym doskonałej pisowni i gramatyki. W dzisiejszych czasach nie jest to trudne, biorąc pod uwagę bogactwo sprawdzania pisowni i sprawdzania gramatyki w edytorach tekstu. Podobnie problemy w stylu kodowania mogą często być zautomatyzowane w odpowiednio inteligentnych edytorach kodów. Nie spodziewałbym się, że zobaczę tego rodzaju rzeczy w przeglądzie kodu; czas każdego jest zbyt cenny.
Robert Harvey
2
arogancka / elitarna recenzja jest łatwa i prawie nie wymaga wysiłku. Aby dokonać przeglądu technicznego, musisz przeczytać i zrozumieć kod, a następnie wymyślić lepsze rozwiązanie ... oznacza to, że musisz pracować. Nie jestem zaskoczony wynikiem twojej propozycji. Powinieneś zorganizować proces przeglądu z mierzalnymi i dobrze zdefiniowanymi zadaniami, aby coś osiągnąć.
JoulinRouge,
2
Jeśli używasz zwinnego, jest to coś, co możesz przywołać przy retrosach bez wskazania jednego celu. Wspomnij, że główną funkcją przeglądów kodu jest poprawność kodu, a nie jego estetyka. W takim przypadku staje się to „koniecznością zmiany” i czymś, co można stale przywoływać, dopóki się nie zmieni.
Kanadyjski Coder

Odpowiedzi:

22

Rodzaje recenzji

Nie ma jednego prawdziwego sposobu przeprowadzania recenzji. Istnieje wiele sposobów oceny, czy kod jest wystarczająco wysokiej jakości. Oczywiste jest pytanie, czy jest on wadliwy, czy ma rozwiązania, które nie skalują się lub są kruche. Kwestie zgodności z lokalnymi standardami i wytycznymi, choć być może nie tak krytyczne jak niektóre inne, są również częścią tego, co przyczynia się do wysokiej jakości kodu.

Rodzaje recenzentów

Tak jak mamy różne kryteria oceniania oprogramowania, tak i osoby oceniające są różne. Wszyscy mamy własne umiejętności i upodobania. Niektórzy mogą myśleć, że przestrzeganie lokalnych standardów jest bardzo ważne, podobnie jak inni mogą być bardziej zainteresowani wykorzystaniem pamięci lub pokryciem kodem twoich testów i tak dalej. Chcesz wszystkich tych rodzajów recenzji, ponieważ jako całość pomogą Ci napisać lepszy kod.

Recenzja to współpraca, a nie gra w tag

Nie jestem pewien, czy masz prawo powiedzieć im, jak mają wykonywać swoją pracę. O ile nie wiesz z całą pewnością inaczej, załóż, że ta osoba stara się przyczynić do tego, co uważa za stosowne. Jeśli jednak widzisz pole do poprawy lub podejrzewasz, że może nie rozumieją, czego oczekuje się od wzajemnej oceny, porozmawiaj z nimi .

Celem wzajemnej oceny jest zaangażowanie twoich rówieśników . Zaangażowanie nie polega na rzucaniu kodu przez ścianę i czekaniu na odpowiedź z powrotem. Zaangażowanie działa razem, aby ulepszyć kod. Weź udział w rozmowie z nimi.

Rada

Pod koniec pytania napisałeś:

jak miałbym zachęcać kolegów do szukania błędów w kodzie w równowadze z rażącymi błędami estetycznymi?

Ponownie odpowiedzią jest komunikacja. Być może możesz ich zapytać: „hej, doceniam to, że łapiesz te błędy. Bardzo by mi pomogło, gdybyś mógł skupić się również na niektórych głębszych kwestiach, takich jak to, czy właściwie tworzę swój kod. Wiem, że to wymaga czasu, ale to naprawdę Wsparcie."

Mówiąc bardziej pragmatycznie, osobiście dzielę komentarze recenzji kodu na dwa obozy i odpowiednio je wyrażam: rzeczy, które należy naprawić, i rzeczy, które są bardziej kosmetyczne. Nigdy nie zapobiegłbym sprawdzeniu stałego, działającego kodu, jeśli na końcu pliku było zbyt wiele pustych linii. Zwrócę jednak na to uwagę, ale zrobię to z czymś takim, jak „nasze wytyczne mówią, że na końcu jest jedna pusta linia, a ty masz 20. To nie jest przeszkoda, ale jeśli masz szansę, może chcieć to naprawić ".

Oto jeszcze jedna rzecz do rozważenia: może to być twój wkurzony zwierzak, że dokonują tak płytkiej weryfikacji twojego kodu. To może być bardzo dobrze, że mnie denerwuje z nich jest to, że (lub jakiś inny kolega, który dostaje podobną opinię) są niechlujstwa w odniesieniu do standardów kodowania własnej organizacji, a to, jak wybrali się komunikować, że z wami.

Co robić po przeglądzie

I na koniec trochę porad po przeglądzie: popełniając kod po przeglądzie, możesz rozważyć zadbanie o wszystkie kosmetyczne rzeczy w jednym zatwierdzeniu, a zmiany funkcjonalne w innym. Mieszanie tych dwóch może utrudnić odróżnienie znaczących zmian od nieznacznych. Wprowadź wszystkie zmiany kosmetyczne, a następnie zatwierdź za pomocą komunikatu „kosmetyczny; bez zmian funkcjonalnych”.

Bryan Oakley
źródło
Dziękuję za doskonałą odpowiedź. Myślę, że moje frustracje leżą tam bigger, gdzie nie rozwiązuje się problemów. Takich jak brakujące indeksy w tabeli DB. Lub próba użycia metodologii lub algorytmu bez zrozumienia uzasadnienia i jako taka niepoprawna. Dla mnie - są one ważniejsze i należy je rozwiązać i rozwiązać przede wszystkim - a estetyka stanowi drugorzędną kwestię.
Sos
1
czy wiesz , że brakuje większych problemów, czy boisz się, że tak się stanie? W przypadku przeoczenia dużego problemu na retrospekcji lub spotkaniu zespołowym możesz poruszyć, że są to rzeczy, które należy wziąć pod uwagę podczas przeglądu kodu. Możesz nawet rozważyć pytanie, kto w zespole skupia się na zmianach w bazie danych, a jeśli nikt nie podniesie ręki, może wyznaczysz kogoś, aby spróbował skupić się tylko na zmianach bazy danych do następnej recenzji.
Bryan Oakley,
Szczerze mówiąc - większość kosmetycznych komentarzy na ogół nie jest kierowana na mój kod. Kiedy przeglądam kod innej osoby, widzę komentarze przeciwko PR dotyczące kosmetyków tuż obok kodu, co uważam za duży problem. Co więcej, większość pierwszego języka zespołu nie jest angielskim. Z mojego punktu widzenia - dziwny błąd w pisowni / gramatyce jest wybaczalny. Nie jestem tutaj, aby ocenić ich użycie angielskiego w komentarzu do bloku dokumentów, ale ich kod.
Sos
2
Cóż, ich komentarze są częścią kodu źródłowego, a jeśli są niepotrzebnie trudne do przeanalizowania, wprowadzające w błąd lub nawet źle, ich w ogóle może być przeszkodą dla każdego, kto później z jakiegoś powodu miał okazję spojrzeć na ten kod. Nie muszą być formalne ani dziełami sztuki, aby osiągnąć swój cel, ale zepsuty angielski, niezależnie od biegłości pisarzy w języku, utrudnia ten cel. Lepiej nie mieć komentarzy niż tych złych.
Deduplicator
1
Większość ludzi docenia, gdy są im wskazywane błędy w języku, aby mogły je poprawić.
gnasher729
7

Ludzie komentują formatowanie kodu i literówki, ponieważ są łatwe do wykrycia i nie wymagają od nich dużego wysiłku.

Ta część jest łatwa do naprawienia - prawie każdy język ma narzędzie do sprawdzania linii lub stylu. Podłącz go do procesu kompilacji, aby nie powiódł się kompilacja, jeśli jest nadmiarowe miejsce. W rezultacie nie będzie żadnych problemów ze stylem do komentowania.

Doprowadzenie ich do znalezienia prawdziwych problemów może być sporym wyzwaniem. Wymaga to nie tylko dociekliwego i zorientowanego na szczegóły sposobu myślenia, ale także znacznej motywacji.

Ta motywacja pochodzi z doświadczenia. Doświadczenie w próbach pracy z kiepskim kodem napisanym przez poprzednich programistów. Starsi inżynierowie mają tego mnóstwo. Pływanie w oceanie gówna sprawia, że ​​nie chcesz już tam wracać.

Gdybym musiał wybrać jedną główną rzecz do sprawdzenia podczas przeglądania kodu, byłaby to łatwość konserwacji kodu. Przez cały czas programista sprawdzający ten kod powinien go zrozumieć i być gotowy do jego ulepszenia i modyfikacji. Jeśli nie ma pojęcia, co się tutaj dzieje, musi to natychmiast powiedzieć, a kod trzeba przepisać.

alex
źródło
Zgadzam się z tobą w sprawie twoich komentarzy. A jeśli zobaczysz to ocean of shitnapisane przeze mnie - zachęcam do zasugerowania przepisania. Ale jeśli zignorujesz, shitale poprosisz mnie o naprawienie kosmetycznych rzeczy - to mnie frustruje.
Sos
4

Oto praktyczna odpowiedź:

Scenariusz 1 : Jesteś starszym członkiem i kimś, kto może zdecydować o praktyce

Zwołaj spotkanie i określ zasady i wytyczne Code Review. Zaufaj mi, jasne wytyczne działają lepiej niż jakikolwiek system lub szkolenie „honorowe”. Wyjaśnij, że nie wolno w ogóle zgłaszać problemów z formatowaniem kodu. Niektórzy członkowie będą się sprzeciwiać. Posłuchaj ich, a następnie poproś, aby postępowali zgodnie z wytycznymi przez pierwsze kilka miesięcy jako eksperyment. Pokaż chęć zmiany, jeśli obecne wytyczne nie działają.

Scenariusz 2 : Nie jesteś kimś, kto decyduje o praktyce lub jesteś stosunkowo młodszym członkiem zespołu

Najlepszym rozwiązaniem jest rozwiązanie problemów z przeglądaniem kodu, dopóki nie uda się osiągnąć scenariusza 1 .

Kshitij Upadhyay
źródło
2

Prostą odpowiedzią, aby zapobiec „trywialnym” komentarzom do recenzji kodu, jest naleganie (ze względu na wydajność), że to recenzent powinien je naprawić. Jeśli więc recenzent stwierdzi, że (horror!) Przegapiłeś przystanek lub źle przeliterowałeś komentarz, powinien to naprawić i przejść dalej.

Z mojego doświadczenia wynika, że:

  • Twoi recenzenci przestają komentować te względnie bezcelowe komentarze i przekazują je z powrotem do naprawy.
  • naprawi je tylko większość recenzentów OCD, co oznacza, że ​​większość twoich recenzji przejdzie. ma to efekt domina polegający na tym, że deweloperzy muszą dokonywać bardziej merytorycznych recenzji, ci, którzy zgłaszają ciekawostki, ponieważ tak naprawdę nie sprawdzają kodu, będą musieli uzasadnić, dlaczego wszystkie ich recenzje przechodzą bez komentarza.

Tak czy inaczej, jest to korzyść. Koszt nieudanej recenzji jest wysoki, jeśli chodzi o zmuszenie programisty do zatrzymania tego, nad czym pracuje, i ponownego odwiedzenia jego kodu, a następnie do następnej recenzji. Jeśli recenzja wykryje prawdziwą jakość kodu lub problemy architektoniczne, ten koszt jest wart każdego grosza, ale zapłacenie tego kosztu za ciekawostki nie jest.

gbjbaanb
źródło
0

Przejrzyj proces recenzji

Oprócz recenzji kodu sugerowałbym regularne sprawdzanie Procesu recenzji. Z pewnością można się tutaj wiele nauczyć, np. Jak poprawnie oceniać kod.

Zwykle niektóre z nich są niedoświadczone i po prostu nie wiedzą, czego szukać. Potrzebują trochę wskazówek nie tylko w odniesieniu do swojego kodu, ale także w zakresie pomocnych recenzji kodu. Przekazywanie opinii na temat recenzji doprowadzi do procesu uczenia się, który (pełny) spowoduje lepsze recenzje i lepszy kod.

Następnie dobrym pomysłem może być (luźne) sformalizowanie zestawu wartości i kryteriów, tego, co organizacja lub zespół postrzega jako Dobry Kod ™ oraz jakich anty-wzorców należy unikać. Nie chodzi o ustawienie czegoś. konkretnie, ale od samego początku uzyskiwania powszechnego konsensusu co do jakości kodu .

JensG
źródło
0

Jako ktoś, kto był po obu stronach tego (recenzowanie kodu napisanego przez innych, a także recenzowanie kodu, który napisałem), powiedziałbym, że mam trzy kategorie, do których każda opinia dotyczy. Po czwarte, jest też zachwycający przypadek „wszystko jest dobrze”.

„Byłoby miło, ale to cię nie zablokuje” (jeśli wszystkie opinie mieszczą się w tej kategorii, mogę wysłać prośbę o scalenie z „będzie scalony w XX: XX, chyba że powiesz mi, że chcesz je naprawić” lub „jasne, śmiało, aby się zameldować, niezależnie od tego, który blok rzuciłby system został wyłączony”):

  • Zapominanie kropek na końcu zdań (w ciągach dokumentów lub komentarzach). Niezdarna gramatyka we wszystkim, co nie jest wysyłane do użytkowników w jakikolwiek sposób w kształcie lub formie (ponownie, ciągi dokumentów i komentarze)
  • Kod, który ma bardziej elegancki sposób, ale co jest zrozumiałe i idiomatyczne (prawdopodobnie wstawię nawet moją sugestię, więc łatwo jest wyjść lub naprawić)

„Rzeczy, które muszą zostać naprawione, ale zaufam ci, że to zrobisz” (jeśli wszystkie rzeczy mieszczą się w tej lub poprzedniej kategorii, odpowiem „napraw to, połączę się, gdy mi powiesz” ponownie zrobione ”(i w tym momencie prawdopodobnie szybko przejrzę, aby zobaczyć, że nic więcej nie wyskoczyło)):

  • Drobne sprzeczki („porównujesz wartość logiczną true, to trochę paranoiczne ...”, ...)
  • Drobne naruszenia stylu („przewodnik po stylu mówi X, to, co zrobiłeś, jest poza tym; zrobiłbym Y lub Z, w zależności od tego, w którą stronę chcesz iść”)
  • Kilka brakujących przypadków testowych, które nie powinny być trudne do napisania

I wreszcie „rzeczy, które są problematyczne, po rozwiązaniu tych problemów będę musiał ponownie sprawdzić kod” (jeśli jest coś w tej kategorii, będzie trzeba przeprowadzić kolejną rundę recenzji, więc dodaj komentarz „ jest też kilka drobnych rzeczy, miło byłoby zobaczyć niektóre z nich naprawione, gdy jesteś przy tym „jeśli jest coś z dwóch pierwszych kategorii):

  • Mogłoby to wyglądać tak: „nie, naprawdę istnieje O wiele lepszy sposób na napisanie tego”, „nie obliczasz tego, czego oczekujesz, ponieważ w teście jednostkowym nie ma takich przypadków skrajnych” i wszystkich innych poważnych problemów.
Vatine
źródło
0

Wygląda na to, że niektórzy zapomnieli o najważniejszym pytaniu: co tak naprawdę chcesz osiągnąć dzięki recenzjom kodu?

Celem przeglądów kodu jest szybsze uzyskanie błędów i łatwego do utrzymania kodu. Przeglądy kodu osiągają to na kilka sposobów: programiści piszą przede wszystkim lepszy kod, ponieważ wiedzą, że zostanie sprawdzony, wiedza jest dzielona w ramach procesu przeglądu, zostaną znalezione błędy, ponieważ recenzent nie jest ślepy na własne błędy jako programiści może być.

Jeśli uważasz, że proces przeglądu jest szansą na odłożenie kolegów lub stworzenie dla nich pracy, oznacza to, że robisz to źle.

gnasher729
źródło