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

Jak wygląda u Was w firmach code review?

2

Code review? A na co to komu? :D
Choć szczerze żałuję, że go nie ma :(

7

To jest w dużej mierze o dojrzałości. Należy sobie odpowiedzieć po co robimy code review. Na pewno nie po to, żeby manifestować „kto ma większego jsona” albo sprawdzać „kropki i przecinki”.

Fajnie, jak na CR pojawiają się również - uwaga - pozytywne komentarze i pochwały. Poza tym dobre CR, to sprawne CR - dopiero kod na produkcji przynosi $$$.

5
Charles_Ray napisał(a):

Poza tym dobre CR, to sprawne CR - dopiero kod na produkcji przynosi $$$.

O, podniosłeś dość ważny punkt.

Mało rzeczy w CR jest tak męczących, jak proszenie o review

dzień....

drugi....

tydzień....

Kiepska sprawa - potem te PRy się piętrzą i ktoś będzie niezadowolony, bo z gotowego PRa zrobi się taki, który trzeba rebase'ować na drugi koniec świata.

Teraz mam duży problem z Gitlabem, bo trzeba się nagrzebać, żeby odnaleźć wszystkie aktualne MRy otwarte w projektach Twojego zespołu. Czemu - nie wiem, może jest na to patent, którego nie znam lub dodatkowa licencja, której nie mam. W Bitbucket po prostu to miałem OOTB i nie musiałem kombinować. Efekt jest taki, że faktycznie przegapiam - ale w normalnej sytuacji staram się po prostu zacząć dzień od sprawdzenia, co jest aktualnie otwarte i zrobić komuś review. Zawsze to jakiś krok w kierunku odgruzowania ;)

3

Owner zadania powinien zabiegać o szybkie review, a jeśli się to nie dzieje - podnosić na retro. To musi chodzić jak w szwajcarskim zegarku, zespół musi być skoncentrowany na celu.

9

Różnie bywało - w jednym projekcie, dzięki CR nabierałem bardzo szybko sprawności w technologii, w drugim miałem do czynienia z neofitami marudzenia wujka Boba i niekończące się dyskusje o tym jak dokładnie ma się nazywać jakaś metoda, wymyślane na zasadzie "widzimisię" zasady, czy lepiej pisać i > 0, czy i >= 1 i inne takie głupoty.

Moim zdaniem CR, żeby działało musi być:

  • robione natychmiast, jak są jakieś CR do zrobienia, to nikt nie ma prawa brać nowego taska
  • obejmować mały kawałek kodu. To jest generalnie problem, że najpierw są za duże taski, później za duże PR i zrobienie CR jest trudne i długie, a ewentualne zmiany po review potrafią wywrócić wszystko do góry nogami
  • robiony porządnie, przynajmniej jeden recenzent musi być fachurą. Nie warto dawać za dużo ludzi do review, bo kończy się to tym, że wszyscy zrobią recenzję na odwal się.

zespół musi być świadomy po co się robi CR, czyli:

  • ostatnia instancja wychwycenia błędów logicznych
  • sprawdzenie jakości testów
  • rozpowszechnienie w zespole wiedzy o różnych obszarach
  • sprawdzenie, czy nowy kod pasuje do ustaleń w projekcie, np. czy endpointy pasują do RESTa, czy ktoś nie wpadł na pomysł użycia Gson, jak cały projekt siedzi w Jackson
  • wychwycenie bałaganu, który jest trudny do wyłapania przez sonara - problemy z dziedziczeniem, naruszenia granic odpowiedzialności, błędny podział na pakiety
7

Ja osobiście nie potrzebuje pochwał w CR i uważam że są raczej zbędne, niemniej nie lubie jak dostaje approve bez słowa, bo wtedy trochę nie ufam, że ktoś serio poważnie na ten kod popatrzył. I w ogóle dziwią mnie trochę komentarze o porównywaniu kto ma większego jsona, bo ja osobiście po to wystawiam review żeby ktoś na ten kod popatrzył i może sie czegoś nauczę. Ba, w moim aktualnym zespole jestem osobą która niejako wprowadziła w ogóle praktykę robienia review :)

Zgadzam się z tym że branche muszą być małe - jak merge ma 50 linijek to będzie 10 sugestii na review, a jak ma 5000 to będzie tylko ok, spoko, approve bo ciężko ogarnąć cokolwiek.

2

Pracuję w 3. firmie i wyglądało to tak:
Firma1: tu masz taski, tu repo, jak zrobisz taska to wrzucasz na developa i robisz kolejnego, 0 CR
Firma2: tu niby lepiej, było CR wykonywane przez typowego seniora przez zasiedzenie któremu nie chciało się za wiele robić, czasem coś skomentował, ale w 90% jak kod działał to było approve bez żadnego słowa.
Firma3: wrzucam pierwszy prosty task, a tu od razu 4 komentarze, a że tu metoda na 10 linii i można rozbić na dwie czytelniejsze, a tu że walimy kilka zapytań do bazy zamiast zrobić jedno i je przeiterować, itp., jestem mega zadowolony bo po tym CR kod jest superczytelny (podobnie jak cały projekt) i nie ma problemu w poruszaniu się w kodzie.

2
  • Marian pyknij mi tą PRke.
  • Done
1

Scroll, scroll, scroll, scroll... a doobra, i tak nie rozumiem co to robi.
To może chociaż przejrzę nadmiarowe spacje i entery.
Approve.

1

CR ? A co to jest ? zasada jest prosta jak sie wypier...oli na produkcji to wtedy wiemy gdzie jest blad xD

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0