Jak znaleźć pozytywne rzeczy w przeglądzie kodu?

184

Po poważnych problemach z jakością w ostatnim roku moja firma niedawno wprowadziła recenzje kodów. Proces przeglądu kodu został szybko wprowadzony, bez wytycznych i jakiejkolwiek listy kontrolnej.

Wybrano mnie i innego programistę do przeglądu wszystkich zmian dokonanych w systemach, zanim zostaną one scalone z bagażnikiem.

Zostaliśmy również wybrani jako „Technical Lead”. Oznacza to, że jesteśmy odpowiedzialni za jakość kodu, ale nie mamy żadnych uprawnień do wprowadzania zmian w procesie, ponownego przypisywania programistów lub wstrzymywania projektów.

Technicznie możemy odrzucić połączenie, przywracając je do rozwoju. W rzeczywistości kończy się to prawie zawsze, gdy nasz szef żąda dostarczenia go na czas.

Nasz menedżer jest MBA, który zajmuje się głównie tworzeniem harmonogramu nadchodzących projektów. Podczas gdy próbuje, prawie nie ma pojęcia, co robi nasze oprogramowanie z biznesowego punktu widzenia, i stara się zrozumieć nawet najbardziej podstawowe wymagania klientów bez wyjaśnienia ze strony dewelopera.

Obecnie rozwój odbywa się w oddziałach programistycznych w SVN, po tym, jak deweloper myśli, że jest gotowy, ponownie przypisuje bilet w naszym systemie biletowym do naszego menedżera. Menedżer następnie przypisuje nam to.

Recenzje kodu spowodowały pewne napięcia w naszym zespole. Zwłaszcza niektórzy starsi członkowie kwestionują zmiany (tj. „Zawsze tak robiliśmy” lub „Dlaczego metoda powinna mieć sensowną nazwę, wiem, co robi?”).

Po kilku pierwszych tygodniach moja koleżanka zaczęła się przesuwać, aby nie sprawiać kłopotów współpracownikom (powiedziała mi, że po zgłoszeniu błędu przez klienta, wiedziała o błędzie, ale obawiała się, że deweloper byłby na nią zły za to, że to wskazała).

Z drugiej strony jestem teraz znany z tego, że jestem dupkiem, który wskazuje problemy z popełnionym kodem.

Nie sądzę, że moje standardy są zbyt wysokie.

Moja lista kontrolna w tej chwili to:

  • Kod się skompiluje.
  • Jest co najmniej jeden sposób działania kodu.
  • Kod będzie działał z większością normalnych przypadków.
  • Kod będzie działał z większością przypadków skrajnych.
  • Kod zgłosi uzasadniony wyjątek, jeśli wstawione dane są nieprawidłowe.

Ale w pełni akceptuję sposób, w jaki udzielam informacji zwrotnej. Podaję już praktyczne punkty wyjaśniające, dlaczego coś należy zmienić, czasem nawet po prostu pytam, dlaczego coś zostało zaimplementowane w określony sposób. Kiedy myślę, że jest źle, zwracam uwagę, że opracowałbym go w inny sposób.

Brakuje mi umiejętności znalezienia czegoś, co można by nazwać „dobrym”. Czytałem, że należy próbować przekazywać złe wiadomości w dobrych wiadomościach.

Ale trudno mi znaleźć coś dobrego. „Hej, tym razem faktycznie popełniłeś wszystko, co zrobiłeś” jest bardziej protekcjonalne niż miłe lub pomocne.

Przykładowy przegląd kodu

Hej Joe,

Mam pytania dotyczące twoich zmian w klasie Library \ ACME \ ExtractOrderMail Class.

Nie rozumiem, dlaczego oznaczyłeś „TempFilesToDelete” jako statyczny? W tej chwili drugie wywołanie „GetMails” wyrzuciłoby wyjątek, ponieważ dodajesz do niego pliki, ale nigdy ich nie usuwasz po ich usunięciu. Wiem, że funkcja jest wywoływana tylko raz na przebieg, ale w przyszłości może się to zmienić. Czy możesz po prostu uczynić go zmienną instancji, wtedy moglibyśmy mieć wiele obiektów równolegle.

... (Niektóre inne punkty, które nie działają)

Drobne punkty:

  • Dlaczego „GetErrorMailBody” przyjmuje wyjątek jako parametr? Przegapiłem coś? Nie rzucasz wyjątku, po prostu przekazujesz go i wywołujesz „ToString”. Dlaczego?
  • SaveAndSend To nie jest dobra nazwa dla metody. Ta metoda wysyła wiadomości z błędami, jeśli przetwarzanie wiadomości nie powiodło się. Czy możesz zmienić nazwę na „SendErrorMail” lub coś podobnego?
  • Nie komentuj starego kodu, usuń go całkowicie. Nadal mamy to w obaleniu.
RobMMurdock
źródło
8
Proszę nie podawać kanapki $ h! T, aby osiągnąć mityczną równowagę między złem a dobrem. Jeśli zrobili coś dobrego, powiedz im, jeśli zrobili coś, co wymaga korekty, daj im znać. Mieszanie dobra i zła osłabia przesłanie. Jeśli otrzymają znacznie więcej negatywnych opinii niż pozytywnych, być może zdadzą sobie sprawę, że muszą się zmienić. Twoje podejście kanapkowe daje stosunek 2: 1 dla każdego negatywu, więc kończą się na dodatnim wyniku netto, to wiadomość, którą chcesz wysłać.
cdkMoose 18.10.16
14
Przestań używać drugiej osoby. Kod jest przedmiotem, a nie koderem. Na przykład napisz: SaveAndSend należy zmienić nazwę, aby lepiej pasowało do jego zachowania, na przykład SendErrorMail . W tej chwili naprawdę wygląda na to, że wydajesz rozkazy swojemu współpracownikowi, nawet z całym „rozkazem, który możesz” rozlałeś na wszystkie strony. Nie zaakceptowałbym tego od recenzenta. Zdecydowanie wolę kogoś, kto wprost mówi „To musi być zrobione”, zamiast prosić mnie (nawet grzecznie) o zrobienie czegoś.
Arthur Havlicek
4
„Czytałem, że należy próbować przekazywać złe wiadomości w dobrych wiadomościach”. Należy upewnić się, że istnieje jasne, globalne zrozumienie, że nie takie są recenzje kodu. Nie przypominają recenzji wydajności pracowników ani recenzji filmu, które ważą się dobrze i źle. Są bardziej jak część procesu kontroli jakości. Nie spodziewałbyś się, że twoi testerzy stworzą bilety z napisem „Ta funkcja jest świetna i działa tak, jak tego oczekuję!”, I nie powinieneś spodziewać się jej również w recenzjach kodu.
Ben Aaronson,
3
Myślę, że twoim pierwszym krokiem powinno być stworzenie podstawowego zestawu standardów / wytycznych kodu, umożliwienie innym członkom wyrażenia opinii i zasadniczo uzyskanie akceptacji / zgody od wszystkich, że wytyczne są „uzasadnione”. Następnie wszyscy są świadomi, że zgodzili się ich przytulać. To działało dobrze w poprzednich. firma, w której pracowałem.
code_dredd,
3
Nie używaj tego wyrażenia „ale w przyszłości może się to zmienić”. Kod tylko dla tego, co jest teraz potrzebne. Nie należy tworzyć złożoności dla przyszłych zmian, które mogą się zdarzyć lub nie. Jeśli na pewno wiesz, że to się zmieni, to jest inne, ale nie dla przypadkowej szansy, że może się zmienić.
House of Dexter,

Odpowiedzi:

124

Jak znaleźć pozytywne rzeczy w przeglądzie kodu?

Po poważnych problemach z jakością w ostatnim roku moja firma niedawno wprowadziła recenzje kodów.

Świetnie, masz prawdziwą okazję do stworzenia wartości dla swojej firmy.

Po kilku pierwszych tygodniach moja koleżanka zaczęła się przesuwać, aby nie sprawiać kłopotów współpracownikom (powiedziała mi, że po zgłoszeniu błędu przez klienta, wiedziała o błędzie, ale obawiała się, że programista byłby na nią zły za wskazanie tego).

Twój współpracownik nie powinien dokonywać przeglądu kodu, jeśli nie jest w stanie poradzić programistom, co jest nie tak z ich kodem. Twoim zadaniem jest znaleźć problemy i rozwiązać je, zanim wpłyną one na klientów.

Podobnie programista zastraszający współpracowników prosi o zwolnienie. Po przeglądzie kodu poczułem się zastraszony - powiedziałem mojemu szefowi i wszystko zostało załatwione. Poza tym lubię swoją pracę, więc podtrzymywałem pozytywne i negatywne opinie. Jako recenzent to mnie nie dotyczy.

Z drugiej strony jestem teraz znany z tego, że jestem dupkiem, który wskazuje problemy z popełnionym kodem.

Cóż, to niefortunne, mówisz, że jesteś taktowny. Możesz znaleźć więcej do pochwały, jeśli masz więcej do znalezienia.

Krytykuj kod, a nie autora

Podajesz przykład:

Mam pytania dotyczące twoich zmian w

Zamiast tego używaj słów „ty” i „twój”, powiedzmy, „zmiany”.

Przegapiłem coś? [...] Dlaczego?

Nie dodawaj retorycznych zawijasów do swoich krytyków. Nie rób też żartów. Słyszę zasadę: „Jeśli sprawia ci przyjemność mówienie, nie mów tego, to nie jest dobre”.

Może wzmacniasz własne ego kosztem kogoś innego. Zachowaj to tylko do faktów.

Podnieś poprzeczkę, przekazując pozytywne opinie

Podnosi poprzeczkę, aby chwalić innych programistów, którzy spełniają wyższe standardy. To oznacza pytanie

Jak znaleźć pozytywne rzeczy w przeglądzie kodu?

jest dobry i wart uwagi.

Możesz wskazać, gdzie kod spełnia idee praktyk kodowania wyższego poziomu.

Szukaj ich, aby postępowali zgodnie z najlepszymi praktykami i wciąż podnosili poprzeczkę. Po tym, jak wszyscy będą oczekiwać łatwiejszych ideałów, przestaniesz je chwalić i poszukasz jeszcze lepszych praktyk kodowania.

Najlepsze praktyki dotyczące języka

Jeśli język obsługuje dokumentację w kodzie, przestrzeniach nazw, obiektowych lub funkcjonalnych funkcjach programowania, możesz wywołać je i pogratulować autorowi używania ich w stosownych przypadkach. Te kwestie zwykle należą do przewodników po stylach:

  • Czy spełnia standardy przewodnika po języku?
  • Czy spełnia najbardziej autorytatywny przewodnik po stylu dla języka (który jest prawdopodobnie bardziej rygorystyczny niż wewnętrzny - a zatem nadal zgodny ze stylem wewnętrznym)?

Ogólne najlepsze praktyki

W różnych paradygmatach można znaleźć punkty do pochwały na temat ogólnych zasad kodowania. Na przykład, czy mają dobre unittests? Czy Unittests obejmują większość kodu?

Szukać:

  • testy jednostkowe testujące tylko przedmiotową funkcjonalność - kpiące z drogiej funkcjonalności, która nie jest przeznaczona do testowania.
  • wysoki poziom pokrycia kodu, z pełnym testowaniem interfejsów API i semantycznie publicznej funkcjonalności.
  • testy akceptacyjne i testy dymu, które testują kompleksową funkcjonalność, w tym funkcjonalność wyśmiewaną do testów jednostkowych.
  • dobre nazewnictwo, kanoniczne punkty danych, więc kod jest SUCHY (nie powtarzaj się), bez magicznych ciągów ani liczb.
  • nazewnictwo zmiennych tak dobrze zrobione, że komentarze są w dużej mierze zbędne.
  • porządki, obiektywne ulepszenia (bez kompromisów) i odpowiednie refaktoryzacje, które redukują linie kodu i dług techniczny, nie czyniąc kodu całkowicie obcym dla oryginalnych autorów.

Programowanie funkcjonalne

Jeśli język jest funkcjonalny lub obsługuje funkcjonalny paradygmat, poszukaj następujących ideałów:

  • unikanie globałów i globalnego państwa
  • za pomocą zamknięć i funkcji częściowych
  • małe funkcje z czytelnymi, poprawnymi i opisowymi nazwami
  • pojedyncze punkty wyjścia, minimalizując liczbę argumentów

Programowanie obiektowe (OOP)

Jeśli język obsługuje OOP, możesz pochwalić odpowiednie użycie tych funkcji:

  • enkapsulacja - zapewnia jasno zdefiniowany i mały interfejs publiczny oraz ukrywa szczegóły.
  • dziedziczenie - kod ponownie odpowiednio wykorzystany, być może poprzez mixiny.
  • polimorfizm - zdefiniowane są interfejsy, być może abstrakcyjne klasy podstawowe, funkcje napisane w celu wspierania polimorfizmu parametrycznego.

w ramach OOP istnieją również SOLIDNE zasady (być może pewna redundancja funkcji OOP):

  • jedna odpowiedzialność - każdy obiekt ma jednego interesariusza / właściciela
  • otwarty / zamknięty - nie modyfikuje interfejsu ustalonych obiektów
  • Podstawienie Liskowa - podklasy można zastąpić wystąpieniami rodziców
  • segregacja interfejsu - interfejsy zapewnione przez kompozycję, być może mixiny
  • inwersja zależności - zdefiniowane interfejsy - polimorfizm ...

Zasady programowania w Uniksie :

Zasady uniksowe to modułowość, klarowność, skład, separacja, prostota, oszczędność, przejrzystość, solidność, reprezentacja, najmniej zaskoczenie, cisza, naprawa, oszczędność, generowanie, optymalizacja, różnorodność i rozszerzalność.

Zasadniczo zasady te można zastosować w wielu paradygmatach.

Twoje kryteria

Są to zbyt trywialne - czułbym się pochlebny, gdyby mnie za to pochwalono:

  • Kod się skompiluje.
  • Jest co najmniej jeden sposób działania kodu.
  • Kod będzie działał z większością normalnych przypadków.

Z drugiej strony są to dość wysokie pochwały, biorąc pod uwagę to, z czym masz do czynienia, i nie zawahałbym się pochwalić programistów za to:

  • Kod będzie działał z większością przypadków skrajnych.
  • Kod zgłosi uzasadniony wyjątek, jeśli wstawione dane są nieprawidłowe.

Zapisujesz zasady przekazywania recenzji kodu?

To świetny pomysł w teorii, chociaż chociaż zwykle nie odrzucałbym kodu z powodu złego nazewnictwa, widziałem tak złe nazewnictwo, że odrzuciłbym kod z instrukcjami, jak to naprawić. Musisz mieć możliwość odrzucenia kodu z dowolnego powodu.

Jedyną regułą, o której myślę o odrzuceniu kodu, jest to, że nie ma nic tak rażącego, że nie chciałbym go produkować. Naprawdę złe imię to coś, co chciałbym powstrzymać od produkcji - ale nie możesz tego zrobić.

Wniosek

Możesz pochwalić stosowanie najlepszych praktyk pod wieloma paradygmatami i prawdopodobnie pod wszystkimi nimi, jeśli język je obsługuje.

Aaron Hall
źródło
8
Twierdziłbym nawet, że wiele z nich może być nagłówkami szablonu opinii na temat przeglądu kodu. Umożliwia to komentarze, takie jak „świetna robota” pod wieloma nagłówkami, bez żadnych dodatkowych kosztów. To również daje kolegom dobry pomysł, w jaki sposób , aby ich kod lepiej.
Stephen
9
Wymieniając wiele dobrych praktyk, prawdopodobnie odpowiadasz na złe pytanie - ponieważ to naprawdę problem XY. I trudno jest znaleźć system recenzji, który pozwoliłby na takie sprzężenia zwrotne. Ważne rzeczy są ukryte w bezużytecznym hałasie. Czasami odpowiedź na pytanie brzmi: „Nie rób tego - to niewłaściwy sposób. Twój problem leży gdzie indziej i powinien zostać odpowiednio rozwiązany”. Jeśli ludzie zaczynają koncentrować się na poszukiwaniu dobrych rzeczy, przegląd kodu stał się stratą czasu. Możesz powiedzieć swojemu współpracownikowi podczas lunchu, jak fajne jest jego wdrożenie i może to docenić.
Eiko
4
@Aaron: Zgadzam się z tobą w podejściu. Wiele odpowiedzi tutaj brzmi: „nie nakładaj cukru”, ale rozumiem, że nie chodzi o zadowolenie wszystkich. Ludzie częściej stosują dobre podejście, gdy dobre rzeczy, które robią, są wzmacniane, a nie wtedy, gdy mówi się im, że się mylą. Kluczem tutaj jest bycie taktownym, ale konsekwentnym w tym, co robić. Z opisu OP jest on w niezbyt doskonałym zespole programistycznym, a nawet starzy członkowie są przyzwyczajeni. Byliby bardziej otwarci na łagodne podejście.
Hoàng Long
@ HoàngLong Nie każdy „stary licznik” niekoniecznie będzie „bardziej otwarty”. Zawsze jest gdzieś ktoś nierozsądny. Na przykład pracowałem z facetem, który nalegał na „przeniesienie” swoich najlepszych praktyk Perla do Pythona i Subversion do Gita, i za każdym razem, gdy był wywoływany, miałam jakąś skargę, nawet jeśli uzasadnienie zostało wyjaśnione. Ponieważ w tym czasie odpowiedzialność za to spoczywała na moich kolanach (byłem jedynym doświadczonym zarówno z Pythonem, jak i Gitem), myślę, że niektórzy ludzie mogą po prostu czuć się zagrożeni (?) I odpowiednio reagować ...
code_dredd
104

Nie zawracaj sobie głowy wybieraniem czegoś dobrego, chyba że jest to solidny zwięzły przykład i nie jest bezpośrednio związany z głównym tematem.

Nie powielę tego - z dźwięków tego masz do czynienia z co najmniej jedną osobą, która nie jest pewna swoich umiejętności i niedojrzale radzi sobie z wyzwaniami związanymi z ich pracą. Prawdopodobnie są również źli w swojej pracy - dobry programista powinien zawsze być skłonny do refleksji, konstruktywnej krytyki i otwartości na zmiany.

Teraz, gdy jest w powietrzu, porozmawiajmy o tobie. Niezależnie od tego, czy uważasz, że jesteś rozsądny, musisz być bardzo ostrożny z ludźmi takimi jak ten, aby uruchomić piłkę. Znalazłem najlepszy sposób na radzenie sobie z tymi ludźmi, aby być bardzo ostrożnym z tym, jak wypowiadasz słowa.

Upewnij się, że obwiniasz kod, a nie autora . Skoncentruj się na jednym z omawianych problemów, a nie na kupie-górze, która jest twoją bazą kodu, którą mogli mieć znaczący udział w tworzeniu i będą postrzegani jako dalszy osobisty atak. Na początku wybierz swoje bitwy, skup się na krytycznych kwestiach, które ujawniają się twoim użytkownikom, aby nie wywoływać salwy krytyki u osoby, która prowadzi ich do odrzucenia wszystkiego, co mówisz.

Język ciała i ton są ważne, jeśli rozmawiasz z nimi osobiście, jasno mów o tym, co mówisz i upewnij się, że nie rozmawiasz z nimi ani nie lekceważysz ich umiejętności technicznych. Najprawdopodobniej od razu staną w defensywie, więc musisz rozwiązać ich zmartwienia zamiast je potwierdzić. Musisz być świadomy tej rozmowy, nie będąc zbyt oczywistym, aby podświadomie myśleli, że jesteś po ich stronie, i miejmy nadzieję, że zaakceptują konieczność wprowadzenia zmian, na które zwrócono uwagę.

Jeśli to nie zadziała, możesz zacząć być bardziej agresywny. Jeśli produkt można uruchomić z sali konferencyjnej, przywołaj go do projektora podczas przeglądu kodu i pokaż błąd z pierwszej ręki, lepiej, jeśli jest tam menedżer, aby osoba nie mogła się wycofać. Nie chodzi o to, aby ich zawstydzić, ale zmusić ich do zaakceptowania, że ​​problem jest prawdziwy dla użytkownika i że należy go naprawić, a nie tylko przeszkodą, jaką wywołuje jego kod.

W końcu, jeśli nigdzie się nie dostaniesz, jesteś zmęczony traktowaniem osoby jak ucznia w przedszkolu, a kierownictwo zupełnie nie zdaje sobie sprawy z problemu i albo źle wpływa na twoją wydajność jako recenzent kodu, albo zależy ci na dobrym samopoczuciu twojego firma i / lub produkt, musisz zacząć rozmawiać z szefem o ich zachowaniu. Bądź tak konkretny i bezosobowy, jak to możliwe - uzasadnij biznes, że cierpi wydajność i jakość.

plast1k
źródło
4
Inną strategią, którą uznałem za przydatną jako recenzent i jako osoba poddawana przeglądowi, jest przypisanie konieczności uwzględnienia przypadków skrajnych z powodu strony trzeciej. Przepraszam osoby na stanowiskach kierowniczych, ale mówię takie rzeczy, jak: „musimy wziąć pod uwagę ten przypadek, ponieważ kierownictwo naprawdę robi wszystko, co w naszej mocy, dlatego chcemy się upewnić, że jest to prawie kuloodporne. Daje im poczucie łatwości”. Wygląda również na to, że zarządzanie nie miałoby nic przeciwko „złemu facetowi” w przypadku PO.
Greg Burghardt,
7
@GregBurghardt Hej, nie bez powodu nazywają to polityką biurową.
plast1k
30
Zgadzam się z tym, co tu mówisz, i dzieje się to jeszcze dalej, ale myślę, że ważne jest, aby pamiętać, że recenzje kodu nie powinny być kontrowersyjne. To dwie osoby siedzące razem, których wspólnym celem jest tworzenie dobrego kodu i dobrego produktu. Czasami nie zgadzasz się co do tego, czy takie podejście jest lepsze, ale wszystkie argumenty po obu stronach powinny być zakorzenione w robieniu właściwych rzeczy dla zespołu, firmy i / lub klienta. Jeśli oboje możecie się zgodzić na to, jest to płynniejszy proces.
hobbs
6
„Nie zawracaj sobie głowy wybieraniem czegoś dobrego, chyba że jest to solidny zwięzły przykład i nie jest bezpośrednio związany z głównym tematem”. - Myślę, że otwarcie jest trochę trudne. Kiedy dokonuję przeglądu kodu, zawsze „zawracam sobie głowę”, aby zacząć od czegoś pozytywnego, nawet ja muszę uciekać się do czegoś łagodnego. Pomaga ustawić ton i pokazuje, że nie szukasz po prostu negatywnych aspektów kodu.
Bryan Oakley,
2
„Upewnij się, że obwiniasz kod, a nie autora”. Zgadzam się, ale niepewny / niedojrzały rodzaj nie przyjmie tego w ten sposób.
MetalMikester
95

Przeglądy kodów mogą być toksyczne, marnować czas, chęć rujnowania żywych wojen. Wystarczy spojrzeć na rozbieżność opinii na temat takich rzeczy, jak czysty kod a komentarze, konwencje nazewnictwa, testy jednostek i integracji, strategie sprawdzania, RESTfulness itp. Itp.

Jedynym sposobem na uniknięcie tego jest zapisanie zasad przekazywania recenzji kodu.

To nie jest osoba, która nie zgodzi się lub nie zatwierdzi meldowania. Sprawdzają jedynie, czy przestrzegano zasad.

A ponieważ są one zapisywane z góry, podczas pisania kodu możesz przestrzegać reguł i nie musisz wyjaśniać swojego rozumowania ani argumentów później.

Jeśli nie lubisz reguł, zwołaj spotkanie, aby je zmienić.

Ewan
źródło
56
„Jedynym sposobem na uniknięcie tego jest zapisanie zasad przekazywania recenzji kodu”. To. Powinieneś przeglądać wszystko pod kątem niektórych standardów, które zostały ustalone dla całego projektu, a nie wbrew osobistym wyobrażeniom o tym, co jest OK, jakkolwiek wnikliwe mogą być twoje osobiste pomysły.
alephzero
6
Pytanie brzmi, jak znaleźć pozytywne rzeczy. Skąd wiesz, że imię jest wystarczająco dobre? Kiedy nazwa jest zbyt słaba, aby przejść przegląd kodu? Wiele rzeczy, które mógł pochwalić, są zbyt subiektywne, aby mieć twarde i szybkie zasady. Jako takie nie sądzę, że to odpowiada na pytanie.
Aaron Hall
20
-1 Uwielbiam sposób, w jaki skaczesz z krytykowania „wojen frajerów”, a następnie mówię „Jedyny sposób, aby tego uniknąć”.
tymtam
33
Niemożliwe jest zapisanie reguły dla każdej możliwej złej decyzji projektowej. A jeśli spróbujesz go stworzyć w trakcie pracy, szybko odkryjesz, że dokument staje się bezużyteczny z samej długości. -1
jpmc26 18.10.16
15
Znacznie bardziej przydatne niż standardy kodowania są programiści i recenzenci, którzy mogą działać jak dorośli.
gnasher729,
25

Nie chciałbym osłaniać twojej opinii, ponieważ można to uznać za protekcjonalne.

Moim zdaniem najlepszą praktyką jest zawsze koncentrowanie się na kodzie, a nigdy na autorze.

To recenzja kodu , a nie opinia programisty , więc:

  • „Ta pętla while może nigdy się nie skończyć”, a nie „Twoja pętla może nigdy się nie skończyć”
  • „W przypadku scenariusza X potrzebna jest walizka testowa”, a nie „Nie napisałeś testu na potrzeby tego scenariusza”

Bardzo ważne jest również stosowanie tej samej reguły podczas rozmowy o recenzji z innymi:

  • „Anne, co sądzisz o tym kodzie?”, A nie „Anne, co myślisz o kodzie Jona?”

Przegląd kodu nie jest czasem na sprawdzenie wydajności - należy to zrobić osobno.

tymtam
źródło
3
W rzeczywistości nie odpowiadasz na pytanie. Pytanie brzmi: „Jak znaleźć pozytywne rzeczy w przeglądzie kodu?” - a ta odpowiedź jest tylko sprzecznością - odpowiadasz: „jak mam wyrazić negatywną opinię”.
Aaron Hall,
15

Dziwi mnie, że do tej pory nie wspomniano w żadnej odpowiedzi i być może moje doświadczenie z recenzjami kodu jest niezwykłe, ale:

Dlaczego przeglądasz całe żądanie scalenia w jednej wiadomości?

Moje doświadczenia z recenzjami kodu są za pośrednictwem GitLab; Zawsze wyobrażałem sobie, że inne narzędzia do sprawdzania kodu będą działać podobnie.

Kiedy daję recenzję kodu, komentuję określone, pojedyncze linie diff. Jest to bardzo mało prawdopodobne, aby zostało odebrane jako osobista krytyka, ponieważ jest komentarzem do kodu - i faktycznie jest wyświetlane jako komentarz do kodu, pokazany bezpośrednio pod kodem, na którym jest komentarzem.

Mogę również komentować całe żądanie scalenia i często tak robię. Ale konkretne punkty można wskazać na konkretnych liniach różnicy. (Ponadto po dodaniu nowego zatwierdzenia w taki sposób, że zmienia się diff, komentarze do „przestarzałego diff” są domyślnie ukryte).

Dzięki temu przepływowi recenzji kodu stają się znacznie bardziej modułowe i mniej spójne. Wiersz z recenzji kodu może po prostu powiedzieć:

Ładne podejście, zawijając to w dedykowaną funkcję!

Lub może powiedzieć

Ta nazwa obiektu tak naprawdę nie pasuje do celu obiektu; może zamiast tego moglibyśmy użyć nazwy takiej jak „XYZ”?

Lub jeśli występują poważne problemy z sekcją, mogę napisać:

Widzę, że ten kod działa (i rozumiem, dlaczego działa), ale wymaga skoncentrowanych badań, aby go zrozumieć. Czy mógłbyś przeredagować go na osobne funkcje, aby łatwiej było utrzymać je w przyszłości?

(Przykład: Funkcja ABC faktycznie robi tutaj trzy rzeczy: bazowanie na foo, blokowanie boz i fripowanie zorfa. Mogą to być osobne funkcje.)

Po napisaniu wszystkich powyższych informacji mogę skomentować podsumowanie całego żądania scalenia, na przykład:

To świetna nowa funkcja, która po połączeniu będzie naprawdę przydatna. Czy możesz wyczyścić nazewnictwo funkcji i obsłużyć refaktoryzację wymienioną w poszczególnych komentarzach, które napisałem, a następnie daj mi znać, aby ponownie na nią spojrzeć? :)


Nawet jeśli prośba o połączenie jest kompletnym śniadaniem dla psa, indywidualne komentarze mogą być proste. Będzie ich po prostu więcej. Następnie komentarz podsumowujący może powiedzieć:

Przykro mi, ale ten kod nie jest tak naprawdę tabaką. Istnieje bardzo wiele przypadków skrajnych (jak wyszczególniono w poszczególnych komentarzach), które będą obsługiwane nieprawidłowo i dadzą złe wrażenia dla użytkownika, a nawet uszkodzenie danych w jednym przypadku. (Patrz komentarz do commit 438a95fb734.) Nawet niektóre normalne przypadki użycia spowodują wyjątkowo niską wydajność aplikacji (specyfika odnotowana w indywidualnych komentarzach do diff dla somefile.c).

Aby była gotowa do produkcji, ta funkcja będzie wymagać pełnego przepisania z większą uwagą na to, jakie założenia są bezpieczne w każdym punkcie przepływu. (Wskazówka: Żadne założenia nie są bezpieczne, chyba że zostały sprawdzone.)

Zamykam żądanie scalenia w oczekiwaniu na pełne przepisanie.


Podsumowanie: Przejrzyj techniczne aspekty kodu jako komentarze do poszczególnych wierszy kodu. Następnie streść te komentarze w ogólnym komentarzu do żądania scalenia. Nie bądź osobisty - po prostu zajmuj się faktami i, według ciebie, kodem , a nie koderem. Opieraj swoją opinię na faktach, dokładnych obserwacjach i zrozumieniu.

Dzika karta
źródło
12

Jestem naprawdę zaskoczony, że nikt tego nie zauważył, ale coś jest nie tak z opublikowaną recenzją próbki.

Po prostu nie ma powodu, aby zwracać się bezpośrednio do Joe. To, że Joe naprawia swoje niepowodzenia, nie jest twoją sprawą. To, że ktoś to robi, to twoja sprawa. Twoja sprawa dotyczy jakości kodu. Więc zamiast pisać prośby / zamówienia / żądania (że gdybym był Joe, mógłbym po prostu odmówić, ponieważ nie jesteś do tego uprawniony), pozostań przy swojej roli i napisz prostą anonimową listę rzeczy do zrobienia.

Próba uczciwego dawania dobrych i złych punktów jest nie tylko niemożliwa, ale całkowicie wykluczona z twojej roli.

Oto przykład przeformułowania z treścią zaczerpniętą z recenzji:

  • Zanim wyciągniemy zmiany w bibliotece Library \ ACME \ ExtractOrderMail Class, musimy naprawić kilka problemów.
  • Chyba, że ​​coś przeoczyłem, „TempFilesToDelete” nie powinien być statyczny.
  • W przyszłości możemy wywoływać tę funkcję więcej niż raz na przebieg, dlatego potrzebujemy (co należy tutaj zrobić).
  • Muszę zrozumieć, dlaczego „GetErrorMailBody” przyjmuje wyjątek jako parametr. (i jestem tu na granicy, bo do tej pory powinieneś już mieć wniosek )
  • Nazwa SaveAndSend powinna zostać zmieniona, aby lepiej pasowała do jej zachowania, na przykład „SendErrorMail”
  • Skomentowany kod należy usunąć w celu zapewnienia czytelności. Używamy subversion do ostatecznego wycofania.

Jeśli sformułujesz taką recenzję, bez względu na to, jak bardzo czytelnik nienawidzi cię osobiście, wszystko, co widzi, to notatki na temat ulepszeń, które ktoś musi później wprowadzić. Kto Kiedy ? Nikogo to nie obchodzi. Co ? Dlaczego ? TO powinieneś powiedzieć.

Zajmiesz się właśnie przyczyną wzrostu napięcia w przeglądach kodu, usuwając czynnik ludzki z równania.

Arthur Havlicek
źródło
Przykładowa recenzja jest ostatnim dodatkiem do pytania, którego większość
respondentów
8

Celem przeglądu kodu jest znalezienie problemów. Jeśli wystąpi jakikolwiek błąd, najlepszą rzeczą, jaką możesz zrobić, to napisać niesprawny test.

Twój zespół (kierownik) powinien komunikować się, że produkcja błędów jest częścią gry, ale znalezienie i je naprawiać uratuje niesłyszących pracę.

Pomocne mogą być regularne spotkania (zespół lub para) i omówienie kilku zagadnień. Pierwotny programista nie wprowadził problemów celowo, a czasem mógł pomyśleć, że był wystarczająco dobry (a czasem nawet może). Ale ta druga para oczu daje zupełnie nowy widok i może się wiele nauczyć od patrzenia na problemy.

To naprawdę jest kwestia kulturowa i wymaga dużo zaufania i komunikacji. I czas na pracę z wynikami.

Eiko
źródło
2
„Cały sens przeglądu kodu polega na znajdowaniu problemów” to prawda - ale żadna z tych odpowiedzi nie odpowiada na zadane pytanie.
Aaron Hall
3
Zadaje
Eiko
1
Twój zespół (kierownik) powinien komunikować się, że produkcja błędów jest częścią gry, ale znalezienie i je naprawiać uratuje niesłyszących pracę. To prawda i oznacza, że ​​każdy jest interesariuszem. Ale nie powinno być obowiązkiem kogoś, kto wskazywał błąd (lub po prostu zły kod spaghetti), aby napisał testowy przypadek, aby udowodnić oryginalnemu koderowi, że jest to błąd. (tylko wtedy, gdy jest on powszechnie kwestionowane, że rzeczywiście jest to błąd.)
Robert Bristow-Johnson
6

Myślę, że pozytywną rzeczą byłoby uzyskanie konsensusu w sprawie standardów kodowania przed dokonaniem recenzji. Inni mają tendencję do kupowania czegoś więcej, jeśli mają wkład.

W przypadku konwencji nazewnictwa zapytaj innych, czy jest to ważne. Większość programistów zgodzi się zwłaszcza wśród swoich rówieśników. Kto chce być osobą, która nie chce się zgodzić z czymś tak powszechnym w świecie programowania? Kiedy zaczynasz proces od wybrania czyjegoś kodu i wskazania wady, stają się bardzo defensywni. Kiedy zostaną ustalone standardy, nie będzie zgody co do interpretacji lub tego, co uważa się za „wystarczająco dobre”, ale jest lepiej niż teraz.

Inną częścią tego procesu jest ustalenie, w jaki sposób recenzje kodu będą obsługiwać zastrzeżenia. To nie może stać się niekończącą się debatą. W pewnym momencie ktoś musi podjąć decyzję. Może może być osoba trzecia, która będzie sędzią, lub recenzent przejmie całą moc. Cel musi być załatwiony.

Ostatnim krokiem jest poinformowanie wszystkich, że recenzje kodu nie były twoim pomysłem, pozostaną, więc każdy powinien dołożyć starań, aby jak najlepiej to wykorzystać. Jeśli wszyscy czują się bezsilni, być może mogą mieć możliwość sprawdzenia Twojego kodu?

Mamy nadzieję, że wymiernym rezultatem dla kierownictwa jest ograniczenie błędów, skarg klientów, opóźnień itp. W przeciwnym razie ktoś właśnie usłyszał modne hasło „przegląd kodu” i stwierdził, że jeśli doda je do procesu, cuda wystąpią bez dużo czasu, energii i włożony w to wysiłek.

JeffO
źródło
4

Może to być trudne, ale nie martw się o dobre opinie, jeśli nie ma nic dobrego do zmierzenia.

Jednak w przyszłości, gdy programiści zaczną ulepszać swój kod, wtedy będziesz chciał przekazać im dobre opinie. Będziesz chciał wskazać ulepszenia w kodzie, a także wskazać korzyści dla całego zespołu. Kiedy zaczniesz dostrzegać poprawę, one również będą i rzeczy powinny powoli zacząć się pojawiać.

Inna rzecz; może być powietrze obronne, ponieważ czują się tak, jakby nie mieli nic do powiedzenia. Dlaczego nie pozwolić sobie na wzajemny przegląd kodu? Zadaj im konkretne pytania i spróbuj je zaangażować. Przeciwnie, nie powinieneś być ty; powinien to być wysiłek zespołu.

  1. Co byś zmienił w tym kodzie, gdybyś miał czas?
  2. Jak ulepszysz ten obszar bazy kodu?

Zapytaj o to teraz i zapytaj o to za sześć miesięcy. Tutaj jest doświadczenie uczenia się.

Główny punkt - nie wychwalaj kodu, jeśli nie jest to uzasadnione, ale rozpoznaj wysiłek i zdecydowanie ulepszenie. Staraj się, aby recenzje były ćwiczeniami zespołowymi, a nie przeciwnymi.

lunchmeat317
źródło
1
„nie martw się o udzielanie dobrych opinii, jeśli nie ma nic dobrego do zmierzenia” Trudno mi uwierzyć, że nie mógł znaleźć czegoś pozytywnego do powiedzenia na temat kodu napisanego przez innych profesjonalnych programistów, wciąż podnosząc poprzeczkę względem oczekiwań wszystkich kod. To nie odpowiada na pytanie - to po prostu sprzeczność.
Aaron Hall,
2
@AaronHall: „Twój kod może być dobrym przykładem, jak nie pisać kodu”. Czy to wystarczy?
gnasher729,
1
@AaronHall Jeśli OP może znaleźć coś pozytywnego do powiedzenia na temat kodu napisanego przez innych profesjonalnych programistów, to na pewno powinien. Jeśli jednak nie ma, nie ma sensu próbować czegoś wymyślić. Zamiast tego PO powinien koncentrować się na wysiłkach programistów i nauce, a nie na samym kodzie.
lunchmeat317
4

Jakość bez napięcia

Pytałeś, jak znaleźć pozytywne rzeczy do powiedzenia na temat kodu, ale Twoim prawdziwym pytaniem jest, w jaki sposób możesz uniknąć „napięć w [twoim] zespole”, rozwiązując jednocześnie „poważne problemy z jakością”.

Stara sztuczka polegająca na umieszczaniu „złych wiadomości w dobrych wiadomościach” może przynieść odwrót. Deweloperzy prawdopodobnie zobaczą to, co to jest: pomysł.

Problemy odgórne w Twojej organizacji

Twoi szefowie powierzyli ci zadanie zapewnienia jakości. Wymyśliłeś listę kryteriów jakości kodu. Teraz potrzebujesz pomysłów na pozytywne wzmocnienie dla swojego zespołu.

Po co pytać nas, co musisz zrobić, aby Twój zespół był szczęśliwy? Czy zastanawiałeś się nad zapytaniem swojego zespołu?

Wygląda na to, że starasz się być miły. Problemem nie jest sposób dostarczania wiadomości. Problem polega na tym, że komunikacja była jednokierunkowa.

Budowanie kultury jakości

Kultura jakości musi być pielęgnowana, aby rosła od podstaw.

Poinformuj swojego szefa, że ​​obawiasz się, że jakość nie poprawia się wystarczająco szybko i chcesz spróbować skorzystać z porad z Harvard Business Review .

Spotkaj się ze swoim zespołem. Modeluj zachowania, które chcesz zobaczyć w swoim zespole: pokora, szacunek i zaangażowanie na rzecz poprawy.

Powiedz coś w stylu: „Jak wiesz, [współpracownik] i ja mieliśmy za zadanie zapewnić jakość kodu, aby uniknąć problemów z produkcją, które ostatnio napotkaliśmy. Osobiście nie sądzę, że rozwiązałem ten problem. Myślę, że moim największym błędem nie było zaangażowanie was wszystkich na początku. [współpracownik] i ja nadal jesteśmy odpowiedzialni przed kierownictwem za jakość kodu, ale idąc dalej, wszyscy wspólnie pracujemy nad jakością. ”

Uzyskaj pomysły od zespołu na temat jakości kodu. (Tablica pomogłaby.) Upewnij się, że twoje wymagania dotrą tam do końca. Przekaż swoje spostrzeżenia z szacunkiem i zadawaj pytania w razie potrzeby. Bądź zaskoczony tym, co Twój zespół uważa za ważne. Bądź elastyczny, bez uszczerbku dla potrzeb biznesowych.

(Jeśli masz jakiś stary kod, który Cię zawstydza, możesz go wypróbować, zachęcając wszystkich do bycia szczerym. Na koniec daj znać ludziom, że go napisałeś. Modeluj dojrzałą reakcję, na którą liczysz, gdy inni otrzymają krytykę konstrukcyjną. )

Recenzje kodów współpracy

Nie pracowałem w miejscu, które opisujesz, gdzie kilku starszych programistów sprawdza cały kod i wprowadza poprawki. Nic dziwnego, że ludzie odpowiadają, jakbyś był nauczycielem, robiąc czerwone ślady na swoich papierach.

Jeśli możesz sprzedać pomysł zarządzaniu, zacznij robić recenzje kodów równorzędnych . Najlepiej zrobić to w małych grupach, w tym ty lub inny odpowiedzialny programista. Upewnij się, że wszyscy są traktowani z szacunkiem.

Ważnym celem recenzowania kodu jest upewnienie się, że kod jest zrozumiały dla wszystkich członków zespołu. Rozważ swój przykład niejasnych nazw funkcji: usłyszenie od młodszego programisty, że uważają, że nazwa funkcji jest myląca, może być łatwiejsze do zaakceptowania niż inna „korekta” od starszego programisty.

Jakość to podróż

Inną rzeczą do zrobienia jest obalenie pojęcia, że ​​przegląd kodu jest czymś pozytywnym / negatywnym. Każdy powinien spodziewać się wprowadzenia pewnych zmian po przeglądzie kodu. (Twoim zadaniem jest dopilnowanie, aby się wydarzyły)

Ale jeśli to nie zadziała ...

Załóżmy, że poczyniłeś pewne kroki w celu ustanowienia kultury jakości. Nadal możesz nie wziąć wszystkich na pokład. Jeśli ktoś nadal nie jest objęty programem jakości, teraz problem polega na tym, że nie pasuje on do zespołu, a nie między wami jest problem. Jeśli będą musieli zostać odwołani z zespołu, reszta zespołu lepiej zrozumie powody.

Tim Grant
źródło
Uważaj na recenzje kodów równorzędnych. Robiliśmy to przez jakiś czas, dopóki nie zdaliśmy sobie sprawy, że młodszy programista zrobił recenzję dla innego młodszego programisty i pozwolił, aby kod przez to nigdy nie powinien przejść. Dwóch seniorów w zespole dokonuje teraz recenzji.
Gustav Bertram,
4

Przepraszam za kolejną długą odpowiedź, ale nie sądzę, aby inni w pełni zajęli się ludzkim elementem tego problemu.

czasem nawet po prostu pytając, dlaczego coś zostało wdrożone w określony sposób. Kiedy myślę, że jest źle, zwracam uwagę, że opracowałbym go w inny sposób.

Problem z „Dlaczego wdrożyłeś go w ten sposób?” polega na tym, że natychmiast stawiasz programistę w defensywie. Samo pytanie może sugerować różne rzeczy w zależności od kontekstu: czy jesteś zbyt głupi, aby wymyślić lepsze rozwiązanie? Czy to najlepsze, co możesz zrobić? Próbujesz zrujnować ten projekt? Czy dbasz nawet o jakość swojej pracy? itd. Pytanie „dlaczego” kod został opracowany w określony sposób, będzie tylko konfrontacyjne, a to obali wszelkie pedagogiczne zamiary, jakie mógł mieć twój komentarz.

Podobnie „zrobiłbym to inaczej ...” jest również konfrontacyjne, ponieważ bezpośrednia myśl dewelopera będzie brzmiała: „ Cóż, zrobiłem to w ten sposób ... Masz z tym problem? ” I znowu, jest bardziej konfrontacyjny niż tak musi być i odwraca dyskusję od ulepszania kodu.

Przykład:

Zamiast pytać „Dlaczego nie użyłeś zmiennej stałej dla tej wartości?”, Po prostu powiedz „Ta zakodowana wartość powinna zostać zastąpiona stałą XYZw pliku Constants.h”. Zadanie pytania sugeruje, że programista aktywnie postanowił nie używać zdefiniowana stała, ale jest całkiem możliwe, że nawet nie wiedzieli, że istnieje. Mając wystarczająco dużą bazę kodu, będzie wiele rzeczy, o których każdy programista nie wie. Jest to po prostu dobra okazja do nauki dla tego programisty w celu zapoznania się ze stałymi projektu.

Z recenzjami kodu jest cienka linia: nie musisz nakładać cukru na wszystko, co mówisz, nie musisz umieszczać złych wiadomości z dobrymi wiadomościami i nie musisz łagodzić ciosu. Może to być kultura, w której się znajduję, ale moi współpracownicy i ja nigdy nie komplementujemy się w recenzjach kodu - części kodu, które tworzymy, które są wolne od wad, są wystarczającym komplementem. Musisz zidentyfikować wadę, którą widzisz, i być może podać powód (dlaczego jest mniej przydatny, gdy jest oczywisty / prosty).

Dobry:

  • „Nazwę tej zmiennej należy zmienić, aby pasowała do naszego standardu kodowania”.

  • „Litera„ b ”w nazwie tej funkcji musi być pisana wielkimi literami, aby być PascalCase.”

  • „Kod tej funkcji nie jest prawidłowo wcięty”.

  • „Ten kod jest duplikatem kodu znalezionego w ABC::XYZ()i powinien zamiast tego użyć tej funkcji.”

  • „Powinieneś używać try-with-resources, aby zagwarantować prawidłowe zamknięcie tej funkcji, nawet w przypadku wystąpienia błędów.” [Dodałem tylko link tutaj, aby użytkownicy spoza Javy wiedzieli, co oznacza try-with-resources]

  • „Tę funkcję należy zmienić, aby spełnić nasze standardy złożoności n-ścieżki”.

Zły:

  • „Myślę, że możesz ulepszyć ten kod, zmieniając nazwę zmiennej, aby pasowała do naszego standardu”

  • Być może lepiej byłoby użyć try-with-resource, aby poprawnie zamknąć rzeczy w tej funkcji”

  • Może być pożądane ponowne przyjrzenie się wszystkim warunkom tej funkcji. Złożoność ścieżki N przekracza maksymalną dozwoloną złożoność naszego standardu”.

  • „Dlaczego wcięcia są tutaj 2 spacjami zamiast 4 naszych standardów?”

  • „Dlaczego napisałeś funkcję, która łamie nasz standard złożoności n-ścieżki?”

W złych stwierdzeniach części pisane kursywą to rzeczy, których ludzie często używają, gdy chcą złagodzić cios, ale wszystko, co naprawdę robi w przeglądzie kodu, sprawia, że ​​wydajesz się niepewny. Jeśli ty, recenzent, nie masz pewności, jak poprawić kod, to dlaczego ktoś miałby cię słuchać?

„Dobre” stwierdzenia są tępe, ale nie oskarżają programisty, nie atakują programisty, nie są konfrontacyjne i nie kwestionują przyczyny wady. Istnieje; oto poprawka. Z pewnością nie są tak konfrontacyjne jak to ostatnie pytanie „dlaczego”.

Shaz
źródło
1
Wiele przykładów, które podajesz, można wykryć za pomocą analizy statycznej. Z mojego doświadczenia wynika, że ​​rzeczy, które pojawiają się w recenzjach kodu, są często bardziej subiektywne i wyrażają opinię: „Zamiast tego nazwałbym XY, ponieważ uważam, że lepiej odzwierciedla to zachowanie”. W naszej organizacji twórca PR może następnie użyć własnego osądu i albo zmienić nazwę, albo pozostawić ją taką, jaka jest.
Muton
@Muton Zgadzam się z tobą w sprawie analizy statycznej. Mamy zautomatyzowane kontrole w projektach, nad którymi pracuję. Posiadamy również narzędzia, które automatyzują formatowanie kodu, więc większość problemów ze stylem kodowania nigdy (lub rzadko) się pojawia. Ale OP specjalnie wspomniało, że ich baza kodu jest bałaganem, więc wyobrażałem sobie, że są to problemy, z którymi mają do czynienia w recenzjach, i myślę, że te proste przykłady pozostają aktualne w odniesieniu do aktualizacji OP stworzonej w celu zaprezentowania przykładowej recenzji.
Shaz,
3

Problem, który widzisz, jest taki: programiści są niezadowoleni, że ich kod jest krytykowany. Ale to nie jest problem. Problem polega na tym, że ich kod nie jest dobry (zakładając oczywiście, że twoje recenzje kodu są dobre).

Jeśli programiści nie lubią krytyki kodu, rozwiązanie jest proste: Napisz lepszy kod. Mówisz, że miałeś poważne problemy z jakością kodu; oznacza to, że potrzebny jest lepszy kod.

„Dlaczego metoda ma mieć sensowną nazwę, wiem, co robi?” jest to coś, co szczególnie mnie niepokoi. On wie, co robi, a przynajmniej tak mówi, ale ja nie. Każda metoda nie powinna mieć po prostu rozsądnej nazwy, powinna mieć nazwę, która natychmiast wyjaśni czytelnikowi kodu, co robi. Możesz odwiedzić stronę Apple i poszukać filmu WWDC na temat zmian z Swift 2 na Swift 3 - ogromna liczba zmian wprowadzonych tylko po to, aby wszystko było bardziej czytelne. Być może tego rodzaju wideo może przekonać programistów, że programiści, którzy są o wiele mądrzejsi, uważają intuicyjne nazwy metod za bardzo, bardzo ważne.

Kolejnym niepokojącym elementem był twój kolega, który powiedział: „powiedziała mi sama, że ​​po zgłoszeniu błędu przez klienta, wiedziała o błędzie, ale obawiała się, że deweloper byłby na nią zły za wskazanie go”. Istnieje możliwość, że istnieją pewne nieporozumienie, ale jeśli deweloper jest zły, jeśli zwrócić uwagę na błąd nich, że jest zły.

gnasher729
źródło
+1 Deweloper może wiedzieć, co robi jego suboptymalnie nazwana funkcja, ale co się stanie, gdy wsiądzie do autobusu?
Mawg
3

Z szacunkiem nie zgadzam się z opisaną przez ciebie metodą przeglądu kodu. Dwaj pracownicy wychodzący do „zamkniętego pokoju” i wychodzący z krytyką instytucjonalizują ten rodzaj kontrowersyjnej koncepcji, której należy unikać dobrych przeglądów kodu .

Sprawienie, by przegląd kodu był pozytywnym doświadczeniem w możliwie najszerszym zakresie, jest niezbędny do jego sukcesu. Pierwszym krokiem jest usunięcie przeciwnego pojęcia przeglądu. Zrób z tego cotygodniowe lub dwutygodniowe wydarzenie grupowe ; upewnij się, że wszyscy wiedzą, że są mile widziani. Zamów pizzę, kanapki lub cokolwiek innego. Nawet nie nazywaj tego „przeglądem kodu”, jeśli wywołuje to negatywne skojarzenia. Znajdź coś do świętowania, zachęć, podziel się - nawet jeśli jest to nic innego jak postęp w bieżącym sprincie lub iteracji. Na zmianę przydzielaj przywództwo nad przeglądem.

Spraw, aby proces ten był staraniem służenia produktowi i ludziom. Jeśli są one wykonywane w sposób konstruktywny, pozytywny, w którym dzielone są dobre techniki lub wzorce i zachęcane tak samo, jak zniechęcane są te słabe - wszyscy odnoszą korzyści. Wszyscy są szkoleni publicznie. Jeśli masz programistę, który nie „rozumie”, należy się tym zająć prywatnie i osobno - nigdy przed szerszą grupą. To jest niezależne od recenzji kodu.

Jeśli masz problem ze znalezieniem czegoś „dobrego” do poruszenia, to rodzi się pytanie: jeśli w projekcie poczyniono postępy, a ludzie pracują, ten postęp sam w sobie jest czymś do świętowania. Jeśli naprawdę nie znajdujesz nic dobrego, oznacza to wszystko, co zostało zrobione, ponieważ ostatnia recenzja jest albo zła, albo nie lepsza niż neutralna . Czy to naprawdę tak jest?

Jeśli chodzi o szczegóły, niezbędne są wspólne standardy. Daj wszystkim udział w tym, co się dzieje. I pozwól, aby nowsze, lepsze techniki zostały zintegrowane z bazą kodu. Nieprzestrzeganie tego gwarantuje, że stare nawyki nigdy nie zostaną wyeliminowane pod pozorem „zawsze tak robiliśmy”.

Chodzi o to, że recenzje kodu są źle traktowane, ponieważ są słabo zaimplementowane, używane jako młoty do umniejszania mniej doświadczonych lub mniej wykwalifikowanych programistów, i to nie jest przydatne dla nikogo. Menedżerowie, którzy używają ich w ten sposób, raczej nie będą bardzo skutecznymi menedżerami. Dobre recenzje kodu, w których każdy jest uczestnikiem, służą wszystkim - produktowi, pracownikom i firmie.

Przepraszam za długość postu, ale będąc w grupie, w której przegląd kodu został w dużej mierze zignorowany, doprowadziło to do spuścizny nieusuwalnego kodu i tylko wąskiego zakresu programistów, którzy byliby w stanie, nawet jeśli pozwolono wprowadzić zmiany w bazie kodu od lat. To była ścieżka, której wolałbym nie pokonywać ponownie.

David W.
źródło
+1 za osobisty przegląd kodu zamiast drogą elektroniczną - to zdejmie krytykę
alexanderbird
3

Paradoks dobrego kodu polega na tym, że nie wyróżnia się on wcale i wygląda na to, że był bardzo prosty i łatwy do napisania. Bardzo podoba mi się przykład gracza w bilard z tej odpowiedzi . W recenzjach kodu sprawia to, że bardzo łatwo jest go przeświecać, chyba że zdarza się, że jest to refaktor przed złym kodem.

Poszukuję tego. Jeśli przeglądam kod i przejrzałem plik, który był tak łatwy do odczytania, że ​​jego pisanie wydaje się zwodniczo łatwe, po prostu wrzucam szybkie „Podoba mi się, że metody tutaj są krótkie i czyste” lub cokolwiek jest odpowiednie .

Powinieneś także dawać przykład. Powinieneś nalegać, aby Twój kod został również przejrzany i powinieneś modelować, jak chcesz, aby twój zespół zachowywał się po otrzymaniu poprawki. Co najważniejsze, należy szczerze podziękować ludziom za opinie. To robi większą różnicę niż jakakolwiek jednostronna informacja zwrotna, którą możesz udzielić.

Karl Bielefeldt
źródło
1

Wygląda na to, że prawdziwym problemem jest to, że tylko dwie osoby wykonują wszystkie przeglądy kodu, z których to tylko Ty traktujesz je poważnie, co doprowadziło cię do niefortunnej sytuacji z dużą odpowiedzialnością i dużą ilością presja ze strony innych członków zespołu.

Lepszym sposobem byłoby rozłożenie odpowiedzialności za dokonywanie przeglądów kodu na zespół i umożliwienie wszystkim udziału w ich wykonywaniu, na przykład poprzez umożliwienie programistom wyboru, komu mają przejrzeć swój kod. Jest to coś, co powinno obsługiwać narzędzie do sprawdzania kodu.

Cześć Żegnaj
źródło
Nie jestem pewien, dlaczego zostało to zanegowane (opinie negatywne bez komentarzy są absurdalnie niemądre w wątku o dobrej recenzji kodu). Wszyscy oceniający powinni być standardową procedurą. Na tablicy Kanbana będzie kolumna do przeglądu kodu, a ktokolwiek w zespole wybierze następny element, powinien dokonać przeglądu (z zastrzeżeniami; nowicjusze nie powinni przez jakiś czas zbierać recenzji, a następnie powinni zacząć od tych, które wymagają niewielka znajomość domeny). Zasadniczo na tablicy scrum: pracuj od prawej do lewej.
Dewi Morgan
1

Widziałem to z pierwszej ręki i chcę odciągnąć cię od odpowiedzi kwestionowanych przez inteligencję emocjonalną - mogą zabijać całe zespoły. O ile nie chcesz rekrutować, szkolić i normalizować nowych programistów każdego roku, tworzenie pozytywnych relacji z rówieśnikami jest niezbędne. W końcu czyż nie ma sensu robić tych recenzji w celu poprawy jakości kodu i wspierania kultury, w której jakość kodu jest przede wszystkim wyższa? Twierdziłbym, że rzeczywiście częścią twojego zadania jest zapewnienie pozytywnego wzmocnienia jako sposobu na zintegrowanie tego systemu przeglądu kodu z kulturą zespołu.

W każdym razie wygląda na to, że musisz przejrzeć system Code Review. W tej chwili z brzmienia wynika, że ​​wszystko w procesie recenzji jest lub może być interpretowane jako subiektywne, a nie obiektywne. Łatwo jest zranić uczucia, gdy wydaje się, że ktoś po prostu rozbiera twój kod, ponieważ go nie lubi, zamiast mieć powód, dla którego może cytować, gdy coś nie pasuje do wytycznych. W ten sposób można łatwo śledzić i „świętować” pozytywną poprawę jakości kodu (w odniesieniu do systemu recenzji) w sposób odpowiedni dla kultury biura.

Koordynatorzy techniczni muszą zebrać się poza sesją przeglądową i wydać wytyczne dotyczące kodowania / listę kontrolną przeglądu kodu, do których należy się stosować i do których należy się odwoływać podczas przeglądu. Powinien to być żywy dokument, który można aktualizować i udoskonalać w miarę rozwoju procesu. Spotkania liderów technicznych powinny odbywać się również wtedy, gdy powinna się odbyć dyskusja „Sposób, w jaki zawsze coś robiliśmy” vs. Gdy wstępna wytyczna zostanie mniej lub bardziej wygładzona, innym sposobem pozytywnego wzmocnienia deweloperów jest poproszenie o opinie, a następnie podjęcie działań w tej sprawie i ewoluuj proces jako zespół, to cuda, aby przyśpieszyć je, aby zacząć sprawdzać kod na pokładzie, więc nie utkniesz w robieniu recenzji, dopóki nie przejdziesz na emeryturę.

nawigator_
źródło
1

Lokal...

Programiści rozwiązują problemy. Jesteśmy uwarunkowani, aby rozpocząć debugowanie, gdy pojawia się problem lub błąd.

Kod jest medium formatu konturu. Przejście do narracji w formacie akapitu w celu uzyskania informacji zwrotnej wprowadza rozłączenie, które musi zostać przetłumaczone. Nieuchronnie coś zgubi się lub źle zrozumie. Nieuniknione jest, że recenzent nie mówi w języku programisty.

Wygenerowane komputerowo komunikaty o błędach rzadko są kwestionowane i nigdy nie są traktowane jako osobisty afront.

Elementy przeglądu kodu to komunikaty o błędach generowane przez ludzi. Mają na celu uchwycić przypadkowe przypadki, których nie zauważają programiści i zautomatyzowane narzędzia, a także nieuniknione skutki uboczne innych programów i danych.

Wnioski ...

Im więcej elementów do przeglądu kodu można włączyć do zautomatyzowanych narzędzi, tym lepiej będą one otrzymywane.

Zastrzeżenie polega na tym, że aby pozostać niekwestionowanym, takie narzędzia muszą być stale aktualizowane, zwykle codziennie lub co tydzień, aby były zgodne z każdą zmianą procedur, standardów i praktyk. Kiedy menedżer lub zespół programistów decyduje się na zmianę, narzędzia muszą zostać zmienione, aby to wymusić. Znaczna część pracy recenzenta kodu staje się wtedy utrzymywaniem reguł w narzędziach.

Przykład opinii dotyczącej przeglądu kodu ...

Przepisać podany przykład, uwzględniając następujące techniki:

  • Przedmiot:

    • Biblioteka \ ACME \ Klasa ExtractOrderMail.
  • Problem podstawowy ...

    • TempFilesToDelete jest statyczny
      • Kolejne wywołania GetMails zgłaszają wyjątek, ponieważ pliki są do niego dodawane, ale nigdy nie są usuwane po usunięciu. Choć teraz jest tylko jedno połączenie, wydajność można by poprawić w przyszłości z pewną równoległością.
      • TempFilesToDelete jako zmienna instancji umożliwiłaby równoległe używanie wielu obiektów.
  • Drugorzędne problemy ...
    • GetErrorMailBody ma parametr wyjątku
      • Skoro sam nie zgłasza wyjątku, a jedynie przekazuje go do ToString, czy jest to konieczne?
    • Zapisz i wyślij nazwę
      • E-mail może, ale nie musi być używany do zgłaszania tego w przyszłości, a ten kod zawiera ogólną logikę przechowywania trwałej kopii i zgłaszania wszelkich błędów. Bardziej ogólna nazwa pozwoliłaby na takie przewidywane zmiany bez zmiany zależnych metod. Jedną z możliwości jest StoreAndReport.
    • Komentowanie starego kodu
      • Pozostawienie komentarza i oznaczenia wiersza OBSOLETE może być bardzo pomocne w debugowaniu, ale „ściana komentarzy” może również ukrywać błędy w sąsiednim kodzie. Nadal mamy go w Subversion. Może tylko komentarz odnoszący się dokładnie do tego, gdzie w Subversion?
DocSalvager
źródło
0

Jeśli nie masz nic miłego do powiedzenia na temat istniejącego kodu (co wydaje się zrozumiałe), spróbuj zaangażować programistę w jego ulepszanie.

W łatce na funkcjonalną zmianę lub poprawkę błędu nie jest pomocne, aby inne zmiany (zmiana nazwy, refaktoryzacja, cokolwiek) były dołączone do tej samej poprawki. Powinien dokonać zmiany, która naprawia błąd lub dodaje funkcję, nic więcej.

Gdzie zmiana nazwy, refaktoryzacja i inne zmiany wskazane, należy je w osobnej poprawki, przed lub po.

  • Jest to dość brzydkie, ale myślę, że jest zgodne z całym naszym innym nieprzyjemnym kodem. Wyczyśćmy to później (w łatce, najlepiej bez zmian funkcjonalnych).

    Pomaga również, jeśli dołączasz do refaktoryzacji i pozwalasz im przejrzeć Twój kod. Daje ci to szansę, by powiedzieć, dlaczego uważasz, że coś jest dobre, a nie złe, a także pokazać przykład dobrze przyjmującego konstruktywną krytykę.

Jeśli chcesz dodać testy jednostkowe do nowej funkcji, dobrze, idą w głównej łatce. Ale jeśli naprawiasz błąd, testy powinny przejść do poprzedniej łatki, a nie przejść pomyślnie, abyś mógł wykazać, że poprawka naprawiła błąd.

Najlepiej byłoby przedyskutować plan (w jaki sposób przetestować poprawkę lub co zrefaktować i kiedy) przed wykonaniem pracy, aby nie włożyli w to zbyt wiele wysiłku, zanim odkryją, że masz z tym problem.

Jeśli okaże się, że kod nie radzi sobie z danymi wejściowymi, które Twoim zdaniem powinny, powinny być również niezgodne z tym, co programista uważa za rozsądne dane wejściowe. Zgadzam się też z góry, jeśli to możliwe. Przynajmniej porozmawiaj o tym, z czym powinien sobie poradzić i jak okropnie może się nie udać.

Nieprzydatny
źródło
Niezależnie od tego, czy w oddzielnych łatach, czy nie, zasadę rozbicia okna należy zaimplementować za pomocą starszego kodu, niezależnie od tego, czy zasadą jest „nie naprawiaj uszkodzonych okien” czy „tylko w [metodach / klasach / plikach?] Dotkniętych przez bieżącą łatkę „. Z mojego doświadczenia wynika, że ​​zapobieganie naprawianiu uszkodzonych okien przez programistów jest toksyczne dla morale zespołu.
Dewi Morgan
1
To prawda, ale zmuszenie ich do naprawienia każdego zepsutego okna w module przed przejściem do zmiany jednej linii jest równie toksyczne.
Bezużyteczny
Zgoda! Wydaje mi się, że polityka wymyślona przez zespół, a nie narzucona z zewnątrz, jest jedynym rodzajem, który może działać.
Dewi Morgan
0

Myślę, że błędem jest założenie, że istnieje techniczne lub łatwe rozwiązanie: „moi współpracownicy są zdenerwowani, gdy oceniam ich pracę według moich standardów i mam moc, by ją egzekwować”.

Pamiętaj, że recenzje kodu to nie tylko dyskusja o tym, co dobre lub złe. Są domyślnie „kimś, kogo używamy”, „który podejmuje decyzję”, a zatem - prawie podstępnie - rangą.

Jeśli nie rozwiążesz tych kwestii, wykonanie recenzji kodu w sposób, który nie napotka problemów, będzie trudne. Jest tu kilka dobrych rad, jak to zrobić, ale postawiłeś sobie trudne zadanie.

Jeśli masz rację, bez żadnego pokoju poruszania się, wskazywanie na to jest atakiem: ktoś się pomylił. Jeśli nie: inny MBA, który go nie otrzymuje. Tak czy inaczej, będziesz złym facetem.

Jeśli jednak to, czego szuka przegląd i dlaczego , jest jasne i uzgodnione, masz przyzwoitą szansę, aby nie być złym facetem. Nie jesteś strażnikiem, tylko korektorem. Uzyskanie tej umowy jest trudnym problemem do rozwiązania [1]. Chciałbym dać ci radę, ale ja sam jeszcze jej nie zrozumiałem ...

[1] Nie rozwiązano tego tylko dlatego, że ludzie powiedzieli „ok” lub przestali się o to kłócić. Nikt nie chce być facetem, który twierdzi, że X po prostu nie jest praktyczny dla naszej {inteligencji, doświadczenia, siły ludzkiej, terminów itp.}, Ale to nie znaczy, że w rzeczywistości chodzi o konkretny przypadek robienia X ...

drjpizzle
źródło
-1

Przeglądy kodu powinny być wzajemne. Wszyscy krytykowali wszystkich. Niech motto brzmi: „Nie złość się, wyrównaj”. Wszyscy popełniają błędy, a gdy ludzie powiedzą, jak naprawić swój kod, zrozumieją, że to zwykła procedura, a nie atak na nich.

Widzenie kodu innych osób jest pouczające. Zrozumienie kodu do tego stopnia, że ​​możesz wskazać jego słaby punkt i musisz wyjaśnić, że słabość może cię wiele nauczyć !

W recenzji kodu nie ma miejsca na pochwały, ponieważ chodzi o to, aby znaleźć wady. Możesz jednak pochwalić poprawki . Można chwalić zarówno aktualność, jak i schludność.

Stig Hemmer
źródło
Nie jestem pewien, dlaczego zostało to zanegowane (opinie negatywne bez komentarzy są absurdalnie niemądre w wątku o dobrej recenzji kodu). Wszyscy oceniający powinni być standardową procedurą. Na tablicy Kanbana będzie kolumna do przeglądu kodu, a ktokolwiek w zespole wybierze następny element, powinien dokonać przeglądu (z zastrzeżeniami; nowicjusze nie powinni przez jakiś czas zbierać recenzji, a następnie powinni zacząć od tych, które wymagają niewielka znajomość domeny). Zasadniczo na tablicy scrum: pracuj od prawej do lewej.
Dewi Morgan
2
@DewiMorgan Nie zgadzam się z stwierdzeniem, że „początkujący nie powinni przez jakiś czas odbierać recenzji”. Nowicjusze dokonujący recenzji to doskonały sposób na zapoznanie się z bazą kodu. To powiedziawszy, nie powinni być jedynym recenzentem! I to powiedziawszy, w każdym razie obawiam się, że przez większość czasu mam tylko jednego recenzenta.
Rozczarowany