Jak wygląda u Was w firmach code review?

Wątek przeniesiony 2021-06-15 17:51 z Java przez Shalom.

0

Zacznę od siebie. 2 approve, żeby zrobić merge'a. Inaczej nie zrobisz. W praktyce wygląda to bardzo słabo. Atmosfera kiepska, dwóch wampirów energetycznych szukających pod lupą not nulli i javadoców;) Im mniej piszesz, tym mniej przypieprzania się. Najlepsze CR mają Ci co uzupełniają insertami słowniki - najzdolniejsi programiści ;) Trochę się odechciewa programowania w robocie. U Was jest lepiej?

2

2 approve, żeby zrobić merge'a. Inaczej nie zrobisz.

Ja nawet nie mam w zespole trzeciego programisty :D

dwóch wampirów energetycznych szukających pod lupą [...] javadoców;)

Po uj komu JavaDoc? o_0"

Najlepsze CR mają Ci co uzupełniają insertami słowniki - najzdolniejsi programiści ;)

Dostajecie według tego premie czy tylko uścisk ręki prezesa?

A teraz o mnie:

  • W poprzedniej firmie mieliśmy 4 przedrostki dla branczy i od tego zależała ilość potrzebnych approve'ów. Najczęstrze zwrotki miałem z powodu niewystarczająco opisowej nazwy. Najbardziej ściąłem się z innym seniorem jak mu powiedziałem że tą niekonkretną nazwę przekopiowałem z jego klasy z jego mikroserwisu. Potem przesuneli mnie do projektu, gdzie byłem pół roku sam :D
  • W aktualnej jest na razie za mały zespół żeby tak cudować :)
0

@KamilAdam: Żle tam nie masz.

Dostajecie według tego premie czy tylko uścisk ręki prezesa?

Jasne :) Moloch, skalowalny scrum, prezes? z nadania pewnie.

1

U mnie wygląda to tak:

  • Pierwsza faza to analiza kodu. Większość mojego zespołu ma >15 lat expa, więc potrafią dużo wydedukować czytając kod, zgadywać (często trafnie) co się wywali, przedstawiają propozycje jak coś zrobić zgrabniej, gdy jest taka potrzeba itd.
  • Druga faza, kiedy już ogarnę wszystko co mi znaleźli w pierwszej fazie to funkcjonalne sprawdzanie fixa/taska. Jak wyjdzie, że coś się wali to też dostaje zwrotkę.\

I w sumie to tyle.

0

@Belka: Brzmi dobrze. W ile osób robicie tę analizę? Analityk też bierze w tym udział?

0

Jesli to nasmiewanki z mojego doboru slow, to poprawie ta "analize" na "czytanie" czy cos podobnego. Powiedz tylko slowo, a zrobie wszystko.

0

@Kiko88: Nie, zupełnie nie. Ciekawość. U nas analityk nie jest na każde skinienie, robią tylko w EA modele.

2

@Kiko88: Review robią inne osoby z teamu po prostu. Nasz produkt należy do niszy bardzo technicznej, nic związanego z fintechem, jakimis webowymi serwisami, więc jeszcze nie miałem styczności z żadnym analitykiem w firmie. Chociaż pewnie gdzieś tam dalej, bliżej krainy managementu istnieją jacyś.

0

@Belka: ok, dzięki.

4

Dwie akceptacje potrzebne do libek, do API tylko jedna.
Recenzent wyszukuje jakich przeoczeń, typu niepotrzebnie wrzucony plik, patrzy czy kod robi to, co powinien, czy nie widać jakichś potencjalnych problemów (typu jakiś nieobsłużony błędny scenariusz), tego typu rzeczy. Sprawdzanie, czy zależność nie jest nullem też trzeba robić, resztę tego typu estetyki sprawdza Sonar.

1

My jesteśmy odważna firma i robimy obrazu commity do mastera/maina build i na produkcję do klienta. Do niedawna narzędzie z którego korzystamy nie wspierało branchy.

8

Tęsknie za Pair Programing.
W jakimś projekcie stosowałem XP i leciał pełny Pair Programing, który pełnił równocześnie rolę Code Review i uważam, że to jedno z sensowniejszych rozwiązań. Kod najłatwiej rozumie się podczas pisania.
Robienie Code Review to najbardziej nudne, niedoceniane i upierdliwe zajęcie, najczęściej robione "na odwal się".
Na dodatek Code Review czasami prowadzi do jakiś dziwnych konfliktów, bo ktoś za bardzo identyfikuje się ze swoim kodem, wiec nie można wprost napisać, że jest źle (ale trzeba "owijać w bawełnę").

8
MarekR22 napisał(a):

ktoś za bardzo identyfikuje się ze swoim kodem, wiec nie można wprost napisać, że jest źle (ale trzeba "owijać w bawełnę").

No jeśli ktoś jest upośledzony w ten sposób, to pair programing z nim musi być prawdziwym masochizmem.

12
somekind napisał(a):
MarekR22 napisał(a):

ktoś za bardzo identyfikuje się ze swoim kodem, wiec nie można wprost napisać, że jest źle (ale trzeba "owijać w bawełnę").

No jeśli ktoś jest upośledzony w ten sposób, to pair programing z nim musi być prawdziwym masochizmem.

Raz trafiłem na takiego nadwrażliwca, który obrażał się o byle co (teraz zawsze go wspominam na rozmowach, jak pada pytanie o sytuacje konfliktowe). Raz obraził się na mnie, bo w Code Review na końcu zdania postawiłem wykrzyknik.

Z mojej obserwacji wynika, że komunikacja za pomocą tekstu pisanego działa na ludzi dziwne i czasami nadinterpretują treść, często do treści technicznej ludzie dokładają ładunek emocjonalny. I nie chodzi o to, że moje komentarze nie są zrozumiałe, bo widzę to wszędzie nawet na wątkach, w których nie biorę udziału.
Najlepszym lekarstwem okazały się rozmowy na Skype lub osobiste i to we wszystkich przypadkach nie tylko o takich jak wspomniany wyżej nerwusek.
Ergo pair progrming jest lekarstwem wyższego poziomu nawet w tej materii.

Wypracowałem zasadę, że jeśli komentarz w review nie prowadzi do szybkiej odpowiedzi: "ok naprawię" lub "jest ok bo ..." tylko do rozpoczyna dyskusję, to dzwonię lub idę do tej osoby.
Daje to bardzo dobre rezultaty. Jeśli komuś w review pojawiają się dyskusje na jednym fragmentem kodu, to polecam tą technikę.

0

@MarekR22: Zawsze komunikacja pisana jest wyzwaniem (szczególnie jak CR daje osoba, z którą nie pracujesz przy projekcie). U nas np. doszedł junior (wiem, że ma bardzo niską stawkę i bardzo silny rys psychopatyczny). I co gościu robi? Napieprza (w większości) bezsensowne komentarze do kodu (javadoc, czasami notnull, że testy inaczej można napisać, bo więcej jeszcze nie da rady) i się odechciewa robić PR, bo trzeba jego wymiociny później czytać. Reakcja tłumu jest taka, że im więcej masz komentarzy (nawet bezsensownych), tym inni mniej chętnie dają approve. Finalnie wymiatacze nie mogą przepchnąć PR. Taka organizacja.

0

@MarekR22
W jakim kontekście pojawił się tam ten wykrzyknik?

2
ToTomki napisał(a):

@MarekR22

W jakim kontekście pojawił się tam ten wykrzyknik?

Już nie pamiętam. Osoba trzecia poinformowała, mnie że gość się poczuł urażony, więc przeczytałem 5 razy wszystkie swoje komentarze w poszukiwaniu przyczyny.
Jak nic nie znalazłem, zadzwoniłem i zapytałem wprost. Gość sam mi powiedział o wykrzykniku i mnie zatkało.

0

@MarekR22: Może Ci trochę empatii zabrakło?

2

Teraz mam bardzo mały zespół (3 osoby, w tym ja i drugi gość jesteśmy nowi) i code review nie jest zbyt drobiazgowe, PRy przechodzą podejrzanie szybko. Lintera do CI dodałem sam bo nie było, mocniejszy branch protection na masterze, żeby nikt czasem czegoś nie popchnął też. Nie mamy wymuszonych przez serwer obowiązkowych approvali (najwyraźniej w Gitlab to ficzer dla hipsterów - nie pytajcie, czemu go nie możemy mieć, bo nie wiem) co czasem się mści, jak komuś myszka zjedzie i kliknie komuś "merge" zamiast "approve".

@MarekR22: na nadmiar ładunku emocjonalnego, obok owijania w bawełnę, mam jeszcze taki patent - nie przypisuję wprost krytyki do autora zmian tzn. unikam zwracania się z krytyką bezpośrednio do autora: zepsułeś, nie przemyślałeś, popraw tamtą linijkę bo jest źle, nie obsłużyłeś edge case'a itd. Chyba działa, albo nie trafiłem jeszcze na turbo-narcyza który się obrazi.

0

@MarekR22: : Nie, nie masz racji. Zwyczajnie proces produkcji oprogramowania słabo działa. Gościu napieprza przez tydzień trudnego taska, po czym jakiś junior publicznie próbuje go zniżyć do parteru, bo javadoc nie dołączył. Powtórzę: proces nie działa.

5

Może gość po prostu nie potrafił zakomunikować powodu swojego zdenerwowania, a wykrzyknik był tylko jakimś tam punktem powiązanym z innymi Twoimi zachowaniami. Nie znam Cię, więc nie chcę oceniać pochopnie, po prostu rzuca mi się w oczy, że teraz piszesz o wykrzykniku w code review (o ile jest możliwość użycia go w kontekście neutralnym, to na ogół jednak wiąże się z negatywnymi emocjami wypływającymi ze strony piszącego i z niefajnymi sytuacjami). Prawdę mówiąc to ja sobie nie wyobrażam użycia wykrzyknika w czasie oceniania kogokolwiek, bo wykrzyknik służy do wyrażania rzeczy dość jednoznacznie nacechowanych, taka kultura, tak samo jak idąc na pogrzeb nie wypada iść w brudnym dresie. Do tego w tym temacie od razu nazwałeś kogoś "nerwuskiem" dlatego, że czuł się zdenerwowany Twoją oceną, a to też nie jest pozytywnie nacechowane słowo. Jesteś pewien, że to on jest nerwowy? Że to on jest źródłem negatywnych emocji w Waszych kontaktach? W innym temacie obruszałeś się nieznajomością podstaw, które Ty akurat znasz, w procesie rekrutacyjnym, nie mówiąc już o tym co sobie myśli osoba, która ma takie braki i chce zwyczajnej rynkowej pensji. Ja sobie nie wyobrażam jak można czymś takim się przejmować. Z mojej perspektywy to świadczy bardziej o Twojej nerwowości, a tutaj opisany przypadek wygląda raczej na negatywną emocjonalnie reakcję osoby, która poddawana jest kontaktom z osobą emocjonalną i wyrażającą negatywne emocje. Żeby też tylko sprawę wyjaśnić - ja tu nie chcę Ciebie oceniać. Ja w ogóle nic o Tobie nie wiem, a opinię wyrobiłem sobie na podstawie tylko trzech Twoich postów, co jest bardzo mocno obciążone błędem i wprost o tym mówię. Nie potrafię Cię ocenić, natomiast jeśli ogólnie w życiu prezentujesz takie podejście jak w tych postach, to może po prostu warto przeprowadzić refleksje nad samym sobą, bo może to nie jest tak, że to inni są winni wszystkiemu, a zwyczajnie nie czują się komfortowo w sytuacji, gdy ktoś do nich podchodzi negatywnie, więc przesadnie (z drugiej strony - może to nie jest przesadna reakcja, a właśnie właściwa reakcja obronna pozwalająca na to, by unikać kontaktu z tym, co wpływa na życie negatywnie) reagują, a ich głównym problemem z Twojej perspektywy jest przede wszystkim to, że nie potrafią Ci zakomunikować faktycznych przyczyn swojego zachowania? Ja to podkreślam jeszcze raz - to jest bardzo obciążona błędem analiza, nie chcę tu Cię oceniać (a już na pewno nie Twoje kwalifikacje techniczne), ale po prostu w życiu widziałem sporo osób, które w życiu podchodziły do innych osób w sposób, który w Twoich postach wydaję się widzieć, co końcowo powodowało bardzo toksyczną i nerwową atmosferę u wszystkich stron biorących udział w interakcji.

5
Kiko88 napisał(a):

po czym jakiś junior publicznie próbuje go zniżyć do parteru, bo javadoc nie dołączył. Powtórzę: proces nie działa.

No jeśli CR polega na wzajemnym zniżaniu do parteru i pokazywaniu sobie, kto ma większego, to problem jest w podejściu a nie w procesie. CR powinno mieć na celu doprowadzenie PR do lepszego stanu, a nie publiczne poniżanie kogoś.

Z drugiej strony, jeśli ktoś każdą krytykę, a w szczególności od osób młodszych stażem odbiera jako próbę upokorzenia, no to nie jest zdrowe podejście.

0

@superdurszlak: Przedstawiłeś bardzo toksyczną sytuację, która faktycznie jest obecna w społeczeństwie. Oczywiście, że seniorzy noszą w spodniach coś na wzór baobabów, ale nawet oni popełniają w kodzie ba(o)bole.

No cóż, grunt to zrozumieć co ma na celu krytyka. Natomiast inną sprawą jest to, że łatwiej ją zrozumieć, gdy jest uargumentowana.

0

@superdurszlak: Szczerze jego komentarze to w większości strata czasu, no nie jest bystrzakiem niestety, a proces jest naprawdę bardzo słaby.

6
  1. Co do konkretnego przypadku. To not nulle javadoce i inne to są rzeczy, które rozwiązuje się na poziomie buildu/CI. Ustawia się z zespołem reguły i wbija w lintera. Do PR takie rzeczy nie dochodzą. Albo inaczej - trafiają max 2-3 razy - jak się coś powtarza i jest na to reguła to leci do lintera. (btw. nie mamy żadnej reguły na javadoce - przeważnie żadnych i to jest raczej norma - o ile nie piszesz biblioteki). Jak nie masz CI i pracujesz z baranami to sam sobie CI zrób. Nawet pluginy do IDE pomogą (choć wsparcie przez IDE to prawie najgorsze rozwiązanie na "code style").
  2. Mimo to code review nie są efektywne - w praktycznie każdej firmie, w jakiej pracuje lub pracowałem - z jednego prostego powodu - za duże. Zadanie ukończone z punktu widzenia biznesu to często za dużo kodu, żeby osoba postronna miała szanse ogarnąć w rozsądnym czasie co się tam działo. Więc na code review łapane są drobnostki, ale nie duże zwały ideologiczne.
4

Ja od pewnego czasu code review robię zupełnie inaczej niż kiedyś. Po pierwsze, nie szukam błędów w kodzie. Ok, czasem bywa że znajdę babola przypadkiem, bo coś rzuci się w oczy. Sprawdzam za to czy są testy i co testują.

Jednak główny nacisk kłade na to czy kod jest łatwy w zrozumieniu i czy wprowadzone abstrakcje mają sens. Także jak widzę nową klasę na 1500 linii i zero komentarza do czego ona służy i gdzie się wpasowuje w całość systemu to często nawet nie czytam tego kodu tylko odwalam i już.

Na styl wciec też nie patrzę bo od tego jest formatter.

2

Wcześniej miałem doświadczenie z bardzo sensownymi CR. Ale teraz mam taki zespół, że bez zastanowienia piszą jak oni by napisali jakaś "linijke" i poza tym, że to co napisali jest bez sensu ale wyglada bardziej fancy to nic nie wnosi. Jeszcze mam drugi przypadek gdzie robię ze starszym niemcem to tam mam komentarze typu zebym nie używał zbyt nowoczesnych konstrukcji jak optional, czy functional interface, forEach bo on tego nie rozumie. Albo jak sobie coś opakuje ładnie w abstrakcje to już muszę walczyć czemu tu zrobiłem jakiegoś małego builderka zamiast 15 ifów w klasie która ma 4k lini xD.
Mam też taki zwyczaj, że jak widzę zakomentowany kod to zazwyczaj go wypieprzam a jak juz widze ze został zakomentowany przed 2015 to nawet nie czytam co tam jest i ostatnio dostałem komentarz zebym tak nie wywalal wszystkie bo jak bedzie potrzebne to nie znajdzie xD (tak używamy gita).

0

@Schadoow: Pierwsza część Twojej wypowiedzi mogłaby również opisywać sytuację u nas. Doładnie chodzi często, że tę linijkę można napisać inaczej. Z reguły dokomentowuję: dróg do celu jest wiele.

0

@MarekR22: Nie ma emocji z mojej strony. Żadnych. Koloryzujesz.

3
Kiko88 napisał(a):

@superdurszlak: Szczerze jego komentarze to w większości strata czasu, no nie jest bystrzakiem niestety, a proces jest naprawdę bardzo słaby.

Jak na CR też mu mówisz, że nie jest bystrzakiem i tracisz na niego czas, to nie zdziw się jak będziesz miał potem konflikty w zespole :)

Skoro czyjś feedback na CR jest taki słaby to można pewnie znaleźć powód inny niż "jest debilem":

  • uważa złą / zbędną praktykę za dobrą, odwrotnie pewnie też
  • skoro tak, to pewnie nie zna tych dobrych praktyk lub nie wie, kiedy jakie stosować
  • może coś mu świta, ale nie umie dobrze uzasadnić swojego zdania
  • jeśli nie zna albo nie umie uzasadniać, to trzeba odesłać do źródeł - do skutku
  • jak zna i nie umie stosować, to pewnie dlatego, że jest juniorem i trudno mieć mu to za złe
  • dopóki wszystkie jego komentarze spławiasz "bo tak" / "bo mnie zniża do parteru", zamiast merytorycznie, to nic się nie zmieni :)

Jak się obrazi o czysto merytoryczne argumenty (bez łatek.w stylu nie jest bystrzakiem!) - to już nie Twój problem, Ty masz czyste konto i podkładkę, że próbowałeś.

Zarejestruj się i dołącz do największej społeczności programistów w Polsce.

Otrzymaj wsparcie, dziel się wiedzą i rozwijaj swoje umiejętności z najlepszymi.