Jak powinno wyglądać code review?

Tak jak się teraz zastanawiam, to to pytanie jest zadane odwrotnie. Pytanie nie powinno brzmieć "jak ma wyglądać Code review"?

Autor powinien zadać pytanie "co daje Code review?" i wtedy spróbować odnieść to do swojej aktualnej sytuacji.

Tak to sobie można dyskutować, wszystko zależy od tego czy to rzeczywiście autor robi wzystko dobrze, a to tylko złośliwie ktoś się do niego czepia bez powodu, czy robi jednak kupę albo z lenistwa albo z braku wiedzy i raczej jest odporny na wiedzę.

To można by było stwierdzić, gdyby podał jakiś konkretny przykład co mu kazano zrobić, ale tego nie mówi.

@Riddle

Z code review nie ma to za wiele wspólnego - nie pełni to funkcji korekt/poprawek, tylko raczej przymuszania do konwencji/rozwiązań (ewentualnie czasem może pełnić też funkcję "mentorowania" że senior blokuje zmiany juniora kiedy mają jakiś oczywisty błąd, ale to rzadko).

W takich wypadkach najczęstsze przyczyny "odrzucenia" PR'a to:

nie dopasowanie się do jakiejś konwencji, stylu
umieszczenie kodu w "złych" miejscach (i mówiąc "złych" mam na myśli w odejściu od aktualnej konwencji)
literówki

Każda z tych wymienionych jest zasadna, jednakże PRa się nie odrzuca, a daje do niego komentarze.

Jeżeli cały kod jest pisany w jakimś stylu, korzysta z jakiegoś konkretnego podejścia, to odejście od niego bez dobrych argumentów nie ma sensu.
Po prostu spowalniasz czytających ten kod, tyle.

Umieszczanie kodu w miejscach innych niż powinny również jest chyba dość oczywiste, tak samo literówki?

Nie rozumiem twojej krytyki konwencji - co do zasady są one dobre, bo przyśpieszają pracę.

Ale pomysł z gate-keepowaniem w organizacji, do którego dostęp mają tylko osoby które podpisały z nami umowę, i mamy do nich zaufanie - raczej nie jest na plus.

Ma sens, bo bycie w organizacji nie implikuje braku błędów czy gorszych dni.

Poziom 0: Pull Requesty -
Obniża zaufanie do innych programistów, budzi poczucie że musimy "pilnować" dostępu przed "złymi zmianami".

Każda zmiana z reguły wymaga istnienia odpowiedniego taska w jirze (co utrudnia wprowadzanie refaktorów lub poprawek nie związanych z taskami)

To już jakieś twoje wymysły, nie są to cechy PRów.

Poziom 1: Mentorowanie,
Zalety:
Reviewerowi łatwiej jest checkoutować kod, i faktycznie go uruchomić na swojej maszynie - to jest ogromny plus na pull requestami (gdzie praktycznie nikt nie checkoutuje kodu

Jeżeli ktoś uznaje że potrzebuje kod przeanalizować lokalnie z IDE to nikt mu przy PRach zrobić tego nie broni, sam tak robię gdy jest coś, co wymaga tego.

Od uruchamiania kodu i testów jest CI, które zrobi to zanim w ogóle reviewer pomyśli o robieniu review.

Poziom 2: Pair programming

Zalety:
Feed-back loop jest praktycznie zerowy, dlatego że review następuje podczas pisania kodu
Lepszy design, dlatego że mentor aktywnie bierze udział w powstawaniu rozwiązania
Bardzo dobry knowledge-sharing
Wady:
Praktycznie żadnych
Jedyną jaka jest to to, że ludzie którzy tego nie próbowali, bardzo się obawiają tego spróbować
Istnieje powszechne przekonanie że pair programming jest wolniejszy niż "normalne developowanie"

Wady:

• strata czasu jeżeli klepany kod jest czymś standardowym/prostym/nudnym/whatever
• łatwiej namówić się na nieprzemyślane rozwiązania, które dopiero po dłuższej analizie, na spokojnie, okazują się nie sprawdzać.

@adamKowal

zapominają dla kogo piszą ten kod i czy dla biznesu lepiej będzie jeśli zostanie dowiezione wcześniej czy metoda zostanie przerobiona pod dyktando seniora dla zwiększenia czytelności DLA programistów jakby programisci byli w tym nawazniejsi. Soft jest pisany dla klienta. Bo ja jestem zorientowany na biznes, pisze dla biznesu przede wszystkim i nie chce poswiecac kolejnego dnia na dostosowywanie kodu, bo panom programistom lepiej będzie się czytać w przyszłości lub będą zadowoleni że jest napisane zgodnie z najnowszymi standardami. Chodzi o to że niektórzy programiści przedkładają idealnosc kodu ponad biznes i zawsze debilnie tłumacza to tym że musi tak być bo przez to cala aplikacja się rozjebie za 5lat

Jak znajdziesz magiczny stosunek (upiększania kodu) do (tworzenia wartości biznesowej), który będzie optymalny, to pewnie mógłbyś jakąś fajną nagrodę dostać, albo taczkę pieniędzy.

Niestety oba podejścia - nieskończone polerowanie kodu, jak i zaciąganie nieskończonego tech debtu mogą spowolnić lub ubić projekt, lub nawet firmę.

Jak określić jakie proporcje przyjąć? nie wiem, prawie każdy pewnie stara się to robić intuicyjnie, na podstawie doświadczenia, analiz czy dyskusji.

Bez kontekstu nie jesteśmy w stanie ocenić czy ci seniorzy robią dobrą robotę.

Może tak jak napisał - @Yarilo - przed zaczęciem kodzenia może powinieneś pogadać o tym jak planujesz to zrobic? idk

lsikora napisał(a):

@adamKowal
Mnie nie śmieszy. Nie raz i nie dwa widziałem, jak mina którą wieszczył reviewer wybucha szybciej niż ktokolwiek się spodziewał. Czsem junior zrobił merge bez poprawiania (jeden mi się taki zdażył w życiu) a czasem była świadoma zgoda na dług techniczny.

Idąc za klasyką psychologii, macie probelm w komunikacji.
Na twoim miejscu, jak czujesz że komentarze są niepotrzebne, to omówiłbym je całym zespołem. I ustalił zasady jak postępować w danej sytuacji. Lub pair programming, żeby wszystkie strony zrozumiały swój sposób myśłenia. Lub prosiłbym o wytłymaczenie do każdego spornego komentarza, dlaczego ma być inaczej i czy zmiana jest wymagana nawet jak opóźni dostarczenie.

Mietek z budowy: robi żeby skończyć szybciej i dostać wypłatę, choćby odwalił fuszerkę to ch0ja mu zrobisz. Jest zrobione i ma być wyplata. Programista nadgorliwiec: Sam kręci na siebie bata, sługa, asceta, dobrowolny niewolnik, doszukuje się co by tu jeszcze poprawić w kodzie, kosztem niedowiezienia sprintu, świr, będzie poprawiał 2 spacje na taby żeby zaspokoić swoje autystyczne wizje. Programiści to największe cioty, które dają się wyzyskiwać kompletnie nie rozumieją jak działa biznes, nie cenią swojego czasu, po pracy nadal klepią kod.
Budowlaniec prawdziwy mężczyzna z krwi i kości - dominujący, ceni siebie, swój czas i jak przychodzi ci zrobić remont na chacie to musisz go sam pilnować, doglądać żeby się nie obijał.
Programista mięczak, virgin boy, drży ze strachu że w kodzie metoda nie jest napisana zgodnie z DRY i jego amerykański Pan się obrazi albo kodzik będzie nieczytelny.
Nie wiem z czego to wynika takie poniżanie się, brak szacunku do swojego czasu, chęć zaimponowania kodem. Podejrzewam że w życiu prywanym są niedowartościowaniu więc dla nich ten kod jest święty, więc mają potrzebę ciagłego samobiczowania się?

Sto razy wolałbym pracować z ekipą takich mentalnych Mietków dla których szef jest złodziejm januszem, którzy zazdroszczą mu że dzięki ich pracy kupuje sobie co miesiąc nowe BMW. To jest zdrowe podejście. Robienie minimum, dbanie przede wszystkim o swój interes, czas i pieniądze.
Praca z takimi nerdami programistami którzy od 10 roku życia spędzają 12 godzin przed komputerem to udręka. Oni skłonni są robić darmowe nadgodziny, bo są podnieceni faktem śledzenia buga w kodzie, dla nich to rozrywka. Jak jesteś gościem z zyciem prywatnym, rodziną, dziećmi który ceni sobie czas, pieniądze, chce mieć życie po pracy to nie wytrzymasz w pracy jako programista bo takich dzieci mentalnych którzy nie myśla pieniędzmi w IT jest na pęczki

LOL

Ty nawet nie rozumiesz, że właśnie jak piszesz gówniany kod to kręcisz na siebie bata. Bo potem przykleja się stopniowo coraz więcej i więcej gówna w miarę upływu czasu a potem się okazuje że już nic nie możesz tam zrobić i okazuje się że już niczego nie dowieziesz z tą różnicą że nic nie możesz na to poradzić, a gdziekolwiek nie dotkniesz nie wiadomo co się popsuje.

Więc robienie czegoś dobrze nie jest w interesie biznesu tylko w twoim własnym, bo to co zrobisz źle teraz zemści się na tobie później.

I dlatego jeśli robisz code review, a przyjdzie ci ktoś, nawali staticów, singletonów czy zmiennych globalnych bo gdyby tego nie zrobił to by nie dowiózł taska, to mimo to odrzucasz i piszesz w review że tak się nie robi

no i właśnie tak się kończy wiara w mityczny zespół, w którym wszystko się razem ustala i dogaduje , ja tam jestem zwolenniczką jakieś hierarchii , oczywiście takiej hierarchii w której with great power comes great reponsibility

Miang napisał(a):

no i właśnie tak się kończy wiara w mityczny zespół, w którym wszystko się razem ustala i dogaduje , ja tam jestem zwolenniczką jakieś hierarchii , oczywiście takiej hierarchii w której with great power comes great reponsibility

A mi zupełnie wszystko jedno czy decyduje zespół, czy hierarchia - ważne, żeby było tak jak ja chcę.

Nie wiem, czy ktoś już to pisał, ale pytanie czy tylko Ty w całym teamie masz taki problem z code review, czy wszyscy tak mają i wszystkich to wkurza, ale każdy siedzi cicho. Jeśli tylko Ty, to raczej pewnie coś rzeczywiście nie halo zwykle z Twoim kodem, natomiast jeśli wszyscy się wkurzają, bo ktoś rzeczywiście wymyśla za bardzo to pogadaj z ludźmi żeby zrobić spotkanie w celu przegadania tego. Zwykle to pomaga.

Chętnie bym poznał opinię drugiej strony bo jak na razie to wygląda jak w sprawie dla nosacza "pani redaktur, przynieśli, my nie przeczytali, podpisali, a tera cza płacić"

Zależy od specyfiki produktu i środowiska pracy. Google zaleca making small progress, bo mają dobrą infrastrukturę na około całego cyklu developmentu i nawet jak coś ktoś zepsuje to nie ma tragedii. Po drugiej stronie spektrum masz aplikacje monolity, gdzie wrzucenie czegoś błędnego na proda to katastrofa.

Niestety nie da się wyrokować bez poznania konkretów