Jak powinno wyglądać code review?

Dlaczego na review ktoś dodaje requesta żeby zrobić cudy na kiju, jak później sprawdzam i marnuje czas to widzę że nie da się tego zrobic bo trzebaby robić z igły widły, jeden komenarz staje się zadaniem na nowego taska. Czy uważacie że jeśli ktoś robi code review to ta osoba proszaca o zmiany powinna zmienić? Mam dosyć, kilka razy senior code nazi chciał poprawki, które wymagają zmiany więcej niż w 2-3 linijakch. Zajmuje mi to pół dnia lub jeden dzień, bo trzeba całą metodą używaną w 10 miejscach przebudować pod zmianę, w innym napisać nową, bo nie będzie się coś zgadzać.

Ja nie mam już siły na dodawanie zmian i robienia tego samego n-tym sposobem. Niech się wykaże jak on wie co ma w glowie to niech zmienia jak mu się nie podoba.

Kilka razy zdażyło mi się nie dowieść sprintu przez zmiany i nie dało się go ugadać ze moje rozwiązanie działa, bo chce tak jak jemu pasuje. Próbowałem mówić mu że to będzie za duża zmiana i jeśli mamy działające moim sposobem, to po co mam kolejny dzień spędzać na przebudowaniu tego samego. Jego odpowiedz: nie narzekaj, to mala zmiana, musimy teraz dodac. Przez takie akcje nie chce mi sie tam pracowac.

Musisz się nauczyć cierpliwości lub umiejętnej komunikacji w celu obrony własnej implementacji. Często jest tak że ktoś sugeruje zmiany nie ot tak, tylko widzi, że da się zrobić lepiej. Czasami jest tak, że zupełnie nie ma racji, ale siłą stanowiska będzie robił swoje.

Jak jest dłuższa dyskusja to calla polecam, bo inaczej będziecie we dwóch mieć poczucie g*wna między Wami - koniec końców i Ty i ten ktoś ma ambicje, żeby to było dobre, więc zgadajcie się i oczyśćcie powietrze, ustalając o co kaman.

Pozdrawiam.

Odpowiadając na pierwotne pytanie: tak, osoba wystawiająca PR wprowadza poprawki z komentarzy, bo to ona jest odpowiedzialna za swój kod i albo go broni, albo akceptuje zmiany, ale ostatecznie się pod tym podpisuje.

A co do drugiej części, to wg mnie problem nie jest z code review tylko z komunikacją w zespole i code review tylko to katalizuje, bo z mojego doświadczenia takie sytuacje rozwiązujemy u nas zwykle tak, że:

1. Jeśli to są małe sprawy, typu dyskusja o nazwę metody, która może by lepiej brzmiała w innej formie, to albo szybka wymiana w komentarzu dlaczego ktoś uważa, że to jest lepsze i zwykle po 2-3 zdaniach dochodzimy do porozumienia

2. Jeśli sugestia zmiany nazwy wynika z tego, że to np. obiekt domenowy gdzie mamy konkretne słownictwo domenowe, albo coś na co mamy "papiery", że takie konwencje mamy przyjęte (np. że DAL nazywamy Repository) to bez gadania zmienia się

3. Jeżeli wprowadzony kod jest poprawny, ZROBIONY ZGODNIE ZE SZTUKĄ/PRAKTYKAMI W PROJEKCIE i NIE BYŁO wcześniej zaprojektowane jak ma to być zrobione (np. senior/podrzędny architekt nie powiedział, że takie sytuacje albo tą konkretną sytuację rozwiązujemy w taki sposób/tym wzorcem/taką biblioteką) to zwykle po potwierdzeniu, że spełnia założenia akceptujemy i ewentualnie dodajemy zdanie techniczne na przeanalizowanie tego obszaru i refactor w najbliższym czasie jeśli np. senior/junior/manager/sprzątacz miał lepszy pomysł na dane rozwiązanie

4. Jeśli wprowadzony kod jest niezgodny ze sztuką, słaby, na odpierdziel, albo wyraźnie było powiedziane jak ten temat ogrywamy (np. było info, że robimy zawsze outbox pattern przy wysyłaniu, dany kod ma być zoptymalizowany pod metrykę X bo jest krytyczny i np. musi być superwydajny nawet jak trzeba trochę więcej linijek napisać, albo mamy zasady, że absolutnie nie mieszamy System.Json i Newtonsoft.Json w projekcie .NETowym) to nie ma zmiłuj - zadanie zrobione źle i wprowadza bałagan, więc do poprawy przez twórcę

I tak jak wspomniałem - jeżeli code review opiera się o przerzucanie wyzwiskami i naciskanie, że ma być po mojemu bo mam 4 certyfikaty z Holiłudu i nie pyskuj, to jest to toksyczne i problem jest w komunikacji zespołu.
Bo albo są jakieś nierozwiązane konflikty (zdarza się, ktoś komuś zabrał ulubionego banana w owocowy czwartek i się poobrażali na siebie na najbliższe 12mc tylko nikt o tym nie wie),
Albo seniorzy/osoby decyzyjne nie potrafią komunikować swoich intencji. I np. mają wizję struktury kodu albo architekturę w głowie, tylko nigdy jej nie spisali, nie pokazali, nie omówili z innymi, żeby każdy rozumiał, a teraz myślą, że każdy im czyta w myślach. I szczerze mówiąc, jako senior nie wyobrażam sobie, że przez rok czy dwa ciągle wszystkie code review muszę poprawiać, bo ktoś pisze kod inaczej niż chciałem.
Od razu, w takim przypadku, wolę spisać dokumentację, nagrać spotkanie gdzie to omawiam, zrobić diagram, albo zrobić umowę zespołową z zasadami kodowania, bo zwyczajnie wtedy mam więcej czasu na opierdzielanie się i wymyślanie nowych głupot, zamiast ciągle pilnować starych.

EDIT po przeczytaniu posta Riddle:
Oczywiście na samym początku warto dodać, że jestem absolutnie przeciwnikiem robienia code review tylko przez seniorów, albo co gorsza konkretnego seniora. Jeżeli inne osoby nie są w stanie sprawdzić typowego kodu, to jedynie oznacza, że mamy burdel, a nie wymianę wiedzy, skoro tylko jeden naznaczony wie jak to powinno być zrobione. Po czas seniorów to warto zapytać jak jest jakiś trudniejszy temat i np. jest code review, w którym są istotne dla całego projektu zmiany, albo jakieś cięższe optymalizacje/usprawnienia. Tak to każdy powinien mniej więcej wiedzieć jaki kod chcemy mieć i powinien móc ocenić naszą pracę i sprawdzić czy nie robimy jakichś błędów logicznych (chociaż te to powinny testy sprawdzić...)

To jest bardzo zawiły temat, i wbrew pozorom nie taki prosty.

Zasadność

Zasadność code review, czyli to żeby na dodawany kod spojrzała więcej niż jedna osoba jest jaknajbardziej na miejscu. Są silne dane które potwierdzają to, że im więcej osób zerknie na kod, tym wyższej jakości z mniejszą ilością bugów będzie. Także tutaj, jak najbardziej 10/10

Gate-keeping

Niestety, prawdą jest, że w firmach często robią się struktury "ważnych" i "ważniejszych", i niektórzy "seniorzy" próbują "blokować" wprowadzanie zmian, np poprzez blokowanie wjazdu do mastera i otwierają go tylko przez pull requesty, pod którym musi się podpisać jeden z seniorów. To jest tzn. "gate keeping", czyli ograniczanie programistom dostępu do kodu, właśnie pod przykrywką "code review" i dobrych praktyk.

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
• rozwiązanie które się nie podoba review'ującemu
• ownership kodu jednej/kilku osób

Czyli inne powody niż powinny wyjść w trakcie faktycznego code review.

Wprowadzenie Code-Review

Są różne sposoby na wprowadzenie code review. Pomysł z Pull Requestami wziął się oczywiście z projektów open-source - kiedy chcieliśmy dać możliwość wprowadzania zmian wszystkim, ale chcieliśmy mieć kontrolę nad tym co wprowadzają (błędy, bugi, trolling, luki w zabezpieczeniach). Więc pomysł z Pull Requestami swoją bazę ma w gate-keepowaniu - i to ma sens, kiedy bronimy się przed każdym możliwym trollem na całym świecie, jak nasze repo jest publiczne.

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.

Alternatywy do wprowadzanie code review:

• Poziom 0: Pull Requesty - każda najmniejsza zmiana wymaga robienia Pull Requesta, i proszenie o review.
  • Wady jakie to ma:
    • Po pierwsze, zbyt duży feedback loop - czasem na review trzeba czekać godziny, jak nie dni
    • Utrudniona komunikacja, bo piszemy w komentarzach zamiast na slacku/face-to-face
    • Review przebiega często przez interfejs webowy (np w diffie na githubie), a prawdziwe code review powinno wyglądać tak że reviewer checkout'uje kod u siebie na lokalu, uruchamia kod, sprawdza czy działa, przechodzi ten sam proces myślowy,
    • Prowadzi do pewnego rodzaju "klapek na oczach", do tego stopnia że często osoby pracujące w ten sposób nawet nie zdają sobie sprawy że "można inaczej".
    • 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)
  • Zalety jakie to ma:
    • Praktycznie żadne
    • Gate-keeping, jeśli można nazwać to "zaletą"?
• Poziom 1: Mentorowanie, tzn. osoba tworzy rozwiązanie, wrzuca go na branch, prosi mentora o sprawdzenie, mentor przegląda zmiany na swoim lokalnym komputerze, i daje okejkę lub nie (np na slacku lub face to face), wtedy junior sam merguje zmiany do mastera. Jeśli junior uzna że jakaś zmiana nie wymaga review, np poprawka w ReadMe.md czy coś takiego, to może zrobić merge'a do mastera w ogóle bez wiedzy mentora.
  • 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)
    • Budzi się większe zaufanie pomiędzy mentorem i juniorem
    • Lepsze na dłuższą metę, jeśli chcemy uczyć juniora
  • Wady:
    • Nie jest tak szybkie jak pair programming
    • Feedback loop jest podobny jak przy pull requestach
• 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"

Dobre code-review

Uwagi na code review - dobre sugestie:

• Ktoś znalazł buga
• Ktoś zauważył kawałek kodu bez testów
• Ktoś zauważył niezgodność z wymaganiem
• Ktoś zauważył dodanie długu technicznego
• Ktoś robi brainstorming pomysłów

Uwagi na "code review", które są złym symptomem:

• reject przez niedopasowanie się do konwencji
• reject przez nie wpasowanie się w styl reviewera
• reject przez arbitralne preferencje reviewera

Konwencje

I tak jak się zgadzam, że konwencje i umówiony jakiś styl kodu, np poprzez (.editorconfig) nie jest złym pomysłem - tak nie sądzę że enforce'owanie go przez code review to najlepszy pomysł. Takie coś powinno wyjść na retro, albo podczas pisania kodu w pair programming, albo w jakiś inny sposób.

Możliwe ze "code review" ma złą nazwę, i nie powinno nazywać się "code review", tylko "solution review".

Brzmi jakbyś miał problemy z komunikacją w zespole. Serio aż tak Cię denerwuje to, że dostajesz poprawki na Code Review?
Nie wiem dlaczego ale po przeczytaniu Twojego posta mam wrażenie, że jesteś problemowym współpracownikiem.

adamKowal napisał(a):

Kilka razy zdażyło mi się nie dowieść sprintu przez zmiany i nie dało się go ugadać ze moje rozwiązanie działa, bo chce tak jak jemu pasuje.

Kody na 300 linii i na 100 mogą biznesowo działać tak samo, ale jednak utrzymaniowo czy pod rozwój w przyszłości to już dwie różne sprawy. (Specjalnie podaje tylko argument z ilością linii, żeby przekazać o co mi chodzi. Tak tak wiem, nie zawsze krócej znaczy lepiej.)

Próbowałem mówić mu że to będzie za duża zmiana i jeśli mamy działające moim sposobem, to po co mam kolejny dzień spędzać na przebudowaniu tego samego.

Teraz działa, ale jak będzie trzeba coś zmodyfikować? Może o to właśnie chodzi temu seniorowi, że twoja wersja będzie gorsza w utrzymaniu?

Przez takie akcje nie chce mi sie tam pracowac.

Czy masz doświadczenie z innymi firmami?

adamKowal napisał(a):

Dlaczego na review ktoś dodaje requesta żeby zrobić cudy na kiju, jak później sprawdzam i marnuje czas to widzę że nie da się tego zrobic bo trzebaby robić z igły widły, jeden komenarz staje się zadaniem na nowego taska. Czy uważacie że jeśli ktoś robi code review to ta osoba proszaca o zmiany powinna zmienić? Mam dosyć, kilka razy senior code nazi chciał poprawki, które wymagają zmiany więcej niż w 2-3 linijakch. Zajmuje mi to pół dnia lub jeden dzień, bo trzeba całą metodą używaną w 10 miejscach przebudować pod zmianę, w innym napisać nową, bo nie będzie się coś zgadzać.

Ja nie mam już siły na dodawanie zmian i robienia tego samego n-tym sposobem. Niech się wykaże jak on wie co ma w glowie to niech zmienia jak mu się nie podoba.

Kilka razy zdażyło mi się nie dowieść sprintu przez zmiany i nie dało się go ugadać ze moje rozwiązanie działa, bo chce tak jak jemu pasuje. Próbowałem mówić mu że to będzie za duża zmiana i jeśli mamy działające moim sposobem, to po co mam kolejny dzień spędzać na przebudowaniu tego samego. Jego odpowiedz: nie narzekaj, to mala zmiana, musimy teraz dodac. Przez takie akcje nie chce mi sie tam pracowac.

Ciężko powiedzieć. Czasami też mnie to drażni, ale czasami widzę, że zrobienie czegoś w jakiś sposób powoduje ciągnięcie się spaghetti. Tyle że najpewniej to ja potem będę je musiał rozwiązywać potem. Sorry, no ale nie zgadzam się na coś takiego, że ktoś będzie dowoził taski i się chwalił czego to nie zrobił, a potem ja podczas debuggowania jakiegoś crasha na produkcji stracę kilka godzin tylko dlatego, że ktoś zrobił coś na odwal. Różnie może być.
I piszę to jako osoba, która (może się mylę, ciężko być sędzią we własnej sprawie) zasadniczo stara się pomagać innym koszmarnie bardziej niż niż dba o swój interes, mimo że po tonie moich postów na forum może się wydawać inaczej.

Trzeba by było spojrzeć na konkretny przypadek, niestety nie da się tak ogólnie odpowiedzieć na to pytanie.

Trudno powiedzieć dokładnie, bo nie wiem co ci owi seniorzy każą ci robić, ale brzmi to tak, że czujesz się bezradny wobec rad seniorów. Nie wiesz czemu, każą ci robić zmiany i nie potrafisz jeszcze wejść w dyskusję na ten temat, lub nawet nie jesteś nastawiony, żeby bronić swojej opinii. Jeśli tak to bardzo prawdopodobne, że masz szczęście, że masz zaangażowanych seniorów (kto wie, może nawet kompetentnych) i dają ci dobrą szansę, żeby się rozwinąć.

Wyczuwam w twoich słowach awersję do dużych zmian w kodzie. Im szybciej się nauczysz pisać kod łatwy w zmienianiu tym lepiej dla Ciebie. Gotowość do przepisania kawałka kodu w razie potrzeby to dobra cecha, daje wiele swobody i spokoju. Oczywiście zawsze trzeba zachować równowagę i wiedzieć czy masz na to czas. Stwierdzenie na review „ok, słuszna uwaga, ale nie dowieziemy tego na czas, dorzućmy to do backloga” jest zupełnie w porządku i im szybciej zaczniesz to robić tym lepiej. Natomiast jeśli senior powie ci, że masz i tak robić to on to bierze na klatę, bo ty go uprzedziłeś, że nie zdążysz. Możesz mieć czyste sumienie.

Umiejętność zaakceptowania czyjejś wizji, nawet jeśli wiesz lepiej, też jest użyteczna. Oszczędza wiele nerwów. Chociaż w tym przypadku chyba raczej lepiej by było gdybyś wykazał więcej zainteresowania, zamiast na ślepo wykonywać polecenia. Dopytaj czemu tak, a nie inaczej. Być może czegoś się nauczysz, a być może zauważysz, że senior sam nie wie czemu (lub dlatego że zawsze tak robił), nie podaje argumentów, nie umie wytłumaczyć – to znak, że być może warto się rozejrzeć za nową pracą.

adamKowal napisał(a):

Dlaczego na review ktoś dodaje requesta żeby zrobić cudy na kiju, jak później sprawdzam i marnuje czas to widzę że nie da się tego zrobic bo trzebaby robić z igły widły

Albo nie rozumiesz co trzeba zrobić

Mam dosyć, kilka razy senior code nazi chciał poprawki, które wymagają zmiany więcej niż w 2-3 linijakch. Zajmuje mi to pół dnia lub jeden dzień, bo trzeba całą metodą używaną w 10 miejscach przebudować pod zmianę, w innym napisać nową, bo nie będzie się coś zgadzać.

To znaczy z że prawdopodobnie wyprodukowałeś kupę i musisz ją teraz posprzątać

Ja nie mam już siły na dodawanie zmian i robienia tego samego n-tym sposobem. Niech się wykaże jak on wie co ma w glowie to niech zmienia jak mu się nie podoba.

Wszędzie jest jakaś hierarchia. Jeśli jesteś juniorem, a review jest robiony przez seniora, to prawdopodobnie słusznie mu się nie podoba a ty masz robić tak tak ci kazano, a nie produkować spaghetti bo działa. Jak on będzie robił sam, to nie potrzeba tam ciebie bo nic nie wnosisz i trzeba cię zwolnić.

Kilka razy zdażyło mi się nie dowieść sprintu przez zmiany i nie dało się go ugadać ze moje rozwiązanie działa, bo chce tak jak jemu pasuje. Próbowałem mówić mu że to będzie za duża zmiana i jeśli mamy działające moim sposobem, to po co mam kolejny dzień spędzać na przebudowaniu tego samego. Jego odpowiedz: nie narzekaj, to mala zmiana, musimy teraz dodac. Przez takie akcje nie chce mi sie tam pracowac.

No to pracuj gdzieś indziej, w czym problem?

No tak to wygląda w rzeczywistosci. Brakuje Wam trzeciej osoby aby weryfikowała też tego seniora, na podobnym poziomie, ale generalnie tak jest. Jeśli dodajesz kod, który jest nieutrzymywalny, niezgodny z ogółem projektu No to dostajesz komentarze takie a nie inne. Niemniej jednak Jeśli faktycznie jego odpowiedź to "dodawaj nie marudz" to jest słabe faktycznie. Powinien uzasadnić i wyjaśnić co Wam daje taka a nie inna implementacja itp. Od tego jest senior, aby rosprzestrzeniac wiedzę, to jedno z jego głównych zadań.

Riddle napisał(a):

To jest bardzo zawiły temat, i wbrew pozorom nie taki prosty.

• Poziom 2: Pair programming
  • 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"

Bylem, widzialem i robilem, wiec sie wypowiem.

- Istnieje powszechne przekonanie że pair programming jest wolniejszy niż "normalne developowanie"

tu sie zgadzam, kod juniora zejdzie szybciej, ale senior nic nie napisze, a swoje podstawowe obowiazki wynikajace z umowy, dzierga za darmo i po godzinach jesli nie chce sie narazic na uwagi biznesu dotyczace jego indywidualnych kompetencji zwiazanych z organizacja wlasnego czasu pracy.
Mniejszy problem jesli pracodawca/klient nie problemow zeby za te nadgodziny zaplacic (chociaz caly czas problem bo to ja powinienem decydowac co robic w czasie wolnym od pracy), natomiast w 10/10 poza pojedynczymi sesjami, ktore de facto i tak nie daja nic juniorowi, senior nie ma szans realizacje poprawnego mentoringu w postaci wspolnego programowania.

- Jedyną jaka jest to to, że ludzie którzy tego nie próbowali, bardzo się obawiają tego spróbować

Tu sie nie zgadzam, probowalem i jestem goracym zwolenikiem wspolnej pracy, w przeciwienstwie do wielu membrow tego forum, caly czas mam zajawke i ogromna satysfakcje jesli chodzi o dzielenie sie swoja wiedza z ludzmi ktorym na tym zalezy, tylko wiesz co, w pracy nikt nie chce mi za to placic wymagajac przede wszystkim realizaji postanowien umowy, a po pracy jakos zaden junior nigdy nie wyrazil zainteresowania

proximus-prime napisał(a):

tu sie zgadzam, kod juniora zejdzie szybciej, ale senior nic nie napisze, a swoje podstawowe obowiazki wynikajace z umowy, dzierga za darmo i po godzinach jesli nie chce sie narazic na uwagi biznesu dotyczace jego indywidualnych kompetencji zwiazanych z organizacja wlasnego czasu pracy.

Jakie podstawowe obowiązki wynikające z umowy?
Jeśli spędził dzień na pair programming, to zrobił już swoją pracę, żadne nadgodziny nie są potrzebne.

proximus-prime napisał(a):

[...] ale senior nic nie napisze, a swoje podstawowe obowiazki wynikajace z umowy, dzierga za darmo i po godzinach jesli nie chce sie narazic na uwagi biznesu dotyczace jego indywidualnych kompetencji zwiazanych z organizacja wlasnego czasu pracy.

No ja też tego nie rozumiem za bardzo. To, że senior nie napisze ani grama kodu, nie oznacza, że nie wypełnia obowiązków wynikających z umowy. Jeżeli jest seniorem to w 95% firm oznacza to, że jego obowiązkiem między innymi jest mentorowanie młodszych, code review, spotkania z biznesem i nie tylko. To właśnie senior najmniej pisze kodu. Taśma produkcyjna obsługiwana jest przez midów / juniorów, senior to wszystko nadzoruje i w wolnym czasie dopiero spokojnie zakodzi jakiegoś taska.
Oczywiście firma firmie nie równa. Jeżeli zespół składa się głównie z seniorów to czasu na kodowanie jest o wiele więcej, głównie kodowanie wtedy i projektowanie wchodzi w grę, bo nie trzeba tłumaczyć podstaw a biznes powinni wszyscy rozumieć na podobnym poziomie. Pair programming też idzie w takich zespołach całkiem przyjemnie (o ile nie ma kwasów i ludzie nie są toksyczni ofc).

Jak zaczynałem pracować jako programista, to rady Seniorów na code-review wręcz mnie trochę wypalały. Pamiętam, że miałem swoją własną wizję jak coś powinno wyglądać i ktoś mi zaburzał to wszystko nawet samym requestem o zmianę nazwę funkcji. Ale jak po jakimś czasie stanąłem po drugiej stronie, to po prostu moment kiedy podczas Pull Requesta pomyślałem sobie "A, dobra, to tylko moje czepialstwo, odpuszczę tym razem" to był moment, którego za każdym razem potem żałowałem. Musi być jakiś dyrygent w tym wszystkim, tylko chyba najcięższym zadaniem jest to, żeby on w odpowiedni sposób to komunikował - otwierał dyskusje, zadawał pytania nakierowujące, tłumaczył wady i zalety proponowanych zmian. Najgorsze co można zrobić chyba, to zostawić proponowany kod w Changes Requested bez komentarza PO CO i DLACZEGO TAK.

somekind napisał(a):

Jeśli spędził dzień na pair programming, to zrobił już swoją pracę, żadne nadgodziny nie są potrzebne.

musrus napisał(a):

... To, że senior nie napisze ani grama kodu, nie oznacza, że nie wypełnia obowiązków wynikających z umowy. Jeżeli jest seniorem to w 95% firm oznacza to, że jego obowiązkiem między innymi jest

Calkiem mozliwe, ze w universum Marvela, w ktorym Wy funkcjonujecie i w ktorym nikogo nie dziwi szop pracz biegajacy z bazuka na pokladzie statku kosmicznego to o czym piszecie jest czyms normalnym natomiast tu na Ziemi gdzie toczy sie normalne zycie sprawy wygladaja zgola inaczej.

proximus-prime napisał(a):

Calkiem mozliwe, ze w universum Marvela, w ktorym Wy funkcjonujecie i w ktorym nikogo nie dziwi szop pracz biegajacy z bazuka na pokladzie statku kosmicznego to o czym piszecie jest czyms normalnym natomiast tu na Ziemi gdzie toczy sie normalne zycie sprawy wygladaja zgola inaczej.

Jestem w stanie uwierzyć że coś takiego nigdy by się nie stało/nie udało w firmie w której pracujesz (i w żadnej innej której pracowałeś), dlatego nazywasz to "nie z tego świata".

Ale są firmy w których taki sposób wytwarzania to codzienność, sam w takiej pracuję.

proximus-prime napisał(a):

Calkiem mozliwe, ze w universum Marvela, w ktorym Wy funkcjonujecie i w ktorym nikogo nie dziwi szop pracz biegajacy z bazuka na pokladzie statku kosmicznego to o czym piszecie jest czyms normalnym natomiast tu na Ziemi gdzie toczy sie normalne zycie sprawy wygladaja zgola inaczej.

Riddle napisał(a):

Ale są firmy w których taki sposób wytwarzania to codzienność, sam w takiej pracuję.

Dokładnie tak. Może nie pracowałem we wszystkich możliwych strukturach, ale przeszedłem przez januszsoft (aka agencja interaktywna), PRLowską firmę softwareową, nowoczesny software house aż pod pracę bezpośrednio w finalnym produkcie. Brakuje mi jedynie pracy w korpo, ale tego wystrzegam się jak ognia póki co. W każdej z tych firm senior był kimś innym, więc jestem w stanie uwierzyć, że Twój świat @proximus-prime jest inny, ale są też inne światy, gdzie senior zajmuje się właśnie rzeczami, o których wspomniałem.

@elwis:

Jeśli tak to bardzo prawdopodobne, że masz szczęście, że masz zaangażowanych seniorów (kto wie, może nawet kompetentnych) i dają ci dobrą szansę, żeby się rozwinąć.

Wyczuwam w twoich słowach awersję do dużych zmian w kodzie. Im szybciej się nauczysz pisać kod łatwy w zmienianiu tym lepiej dla Ciebie. Gotowość do przepisania kawałka kodu w razie potrzeby to dobra cecha, daje wiele swobody i spokoju.

Tak denerwuje mnie jak taki senior pseudo expert półbóg szuka tylko do czego się doczepic żeby się wykazać i pokazać młodemu kto tu rzadzi. Bez zastanowienia wpieprza swoje uwagi czy potrzebne czy nie aby Jego nie obchodzi to że feature opóźni się i kto później będzie się z tego spowiadał? Ja. Przejrzy kod w 5 minut zeby doszukiwać się do czego by się doczepic. Jego uwagi i wyssane z d**y kryteria również nie pozostają bez wpływu na projekt. Z niego jest zdjęta odpowiedzialność. Może sobie zazyczyc cudu i to na mnie będzie spływała odpowiedzialność że nie spełniłem zadania księcia i mam rozczeniowa postawę bo nie ma tego co sobie zażyczył.

Pracowałem z takim typem którego requestow nie można było kwestionować bo zaraz oskarżał o bycie rozczeniowym . A na pytania żeby rozwinął o co mu chodzi konkretnie w tej zmianie dlaczego uważa że jego sposobem jest lepiej - nie dyskutuj. Dodawał uwagi w których nie wiadomo o co mu chodzi. Dopytywałem się jego to odbierał to jako kwestionowanie jego zdania a chciałem tylko żeby wytłumaczył po kolei co ma być zrobione. Do tego też się nie palił, wywracał oczami podirytowany ze jak ktoś śmie nie umiec czytac mu w myślach.
Największy toksyna.

Wedlug mnie należy dodawać ostrożnie uwagi i jeśli są naprawdę konieczne i przemyślane, a nie nawalic uwag jak najwięcej żeby pokazać temu młodemu że jest do niczego i jeszcze wtedyptywc go ze jak śmie kwestionować uwagi seniora. Typek nie skompilował mojego kodu, nie przetestował, nie przyjrzał się bliżej tylko machnął w 5 min uwagi dla zasady, bo jak review nie ma uwag to co to za review?

Napisanie code review bez zastanowienie zajmuje czasem 5 min, zmiany jak są wyssane z d**y to i dzień, dwa można robić.

Nie wspominając już że psychicznie obciążające jest wymyślanie koła na nowo pod wymagania.

Niektorzy seniorzy uważają się za polbogow dla nich liczy się tylko kod, 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 , a inni programiści którzy będą czytac bidulki się nie połapią.

Przyjmuje to forme pewnego rodzaju faszyzmu który niektórym programistom naprawdę wszedł na łeb. W każdej zmianie wieszcza czarne scenariusze, dlatego że nie będzie zmienione za jakiś czas projekt sie rozjebie. Bardzo mnie to śmieszy.

@adamKowal: niestety tak jest czasami. W IT pracuje dużo fajnych ludzi, ale zdarzają się też ultraparapety niestety

Coś mi się wydaje, że ktoś tu zaczyna za bardzo gwiazdorzyć. Kilka już razy widziałem juniorów z takim podejściem i koniec końców zawsze okazywało się, że wynika to z lenistwa - zrobić kupę byle jak, byle szybciej albo z tego, że ktoś się zwyczajnie nie nadawał do programowania i nie był w stanie swoim rozumem objąć czemu robi coś źle oraz jak to powinno być zrobione i dlaczego tak - albo tępota albo lenistwo, zawsze jedno z tych 2.

Może w tym przypadku jest inaczej, ale według mnie jest to bardzo mało prawdopodobne.

@adamKowal w swoim ostatnim poscie poruszasz dwie wazne, acz zupelnie rozlaczne kwestie.
Pierwsza z nich, to trauma wywolana koniecznoscia funkcjonwania w zespole w ktorym masz do czynienia z seniorem-dzbanem, ktory potrafi zdemolowac nawet najsliniejszy juniorski mental i motywacje.
Wszystko o czym piszesz zdarza sie nadwyraz czesto i rownie czesto stajesz przed podjeciem decyzji nie czy zostac czy odejsc. Ostatecznie zawsze odejdziesz, ale czy bedziesz w lepszym czy gorszym stanie psychicznym :) to juz zalezy od Ciebie. Ja w 10 przypadkach na 10 wybieralem, nienaruszony stan psychiczny i satysfakcje z faktu ze zanim oposcilem konkretne miejsce kazdy uslyszal ode mnie to co mialem mu do powiedzenia.

Druga kwestia to kod. Z mojego punku widzenia jest to delikatnie mowiac niewlasciwe podejscie. Kod bardzo wiele mowi o jego tworcy i jeslI jest napisany we wlasciwy sposob pozwala juniorowi szybko sie wdrozyc w projekt, a rownoczesnie duzo nauczyc w ograniczonym zakresie czasu. Nie zgadzam sie z opinia iz kod jest dla biznesu, dla biznesu jest jedynie rezultat jego dzialania, natomiast dla Ciebie i innych programistow ktorzy przejma Twoje zadania, lub beda pracowac razem z Toba, najwazniejsza jest jego jakosc, Twoj komfort oddania taska na czas jest tak samo wazny jak komfort innego programisty ktory tego taska bedzie poprawiac lub rozwijac.

Ostatecznie przyjmuje, ze tak naprawde to przemawia przez Ciebie, z mojego punktu widzenia jak najbardziej uzasadniona frustracja, zwiazana z koniecznoscia wspolpracy, czy wrecz zaleznoscia od tego czy innego "krutkiego wacka" ktory kompensuje sobie swoje kompleksy w taki sposob jaki opisales. Mi "za juniora" tez zdarzalo sie robic poprawki prostego taska po code review taki elementow, ktorzy potrafiliy na kolejnych review poprzednich review pisac za kazdym razem coraz dlluzsze elaboraty na temat gownianej jakosci mojego kodu. Sam fakt iz nie potrafili tego zrobic raz a dobrze, swiadczyl tylko o tym ze tacy z nich byli seniorzy jak z koziej d...py traba. Nie ma sie co ciskac i probowac kopac sie z koniem, tylko trzeba mu wygarnac to co Ci lezy na watrabie i szukac innego zajecia. W dobie wszechobecnej kultury pracy "wlazic w d...pe jak najglebiej tym co moga wiecej" nie ma trzeciej opcji. Trzeba znalezc swoje miejsce, nawet jesli jest mniej korzystne finansowo, trzymac sie tego miejsca i dbac o nie jak o swoje. I co najwazniejsze nie kompensowac swoich paskudnych doswiadczen podobnym zachowaniem w stosunku do innych.

adamKowal napisał(a):

[...]
Przyjmuje to forme pewnego rodzaju faszyzmu który niektórym programistom naprawdę wszedł na łeb. W każdej zmianie wieszcza czarne scenariusze, dlatego że nie będzie zmienione za jakiś czas projekt sie rozjebie. Bardzo mnie to śmieszy.

Cóż są tacy ludzie, z pewnością wielu. Jednak to w jaki sposób przedstawiasz sytuację sugeruje, że po twojej stronie jest część problemu. W tym co piszesz jest wiele gniewu i oburzenia czyli uważasz, że to oni naruszają twoje granice, a z Tobą jest wszystko w porządku (nie zauważyłem choćby zdania zwątpienia w to). Nawet jeśli w rzeczy samej robisz dobrą robotę, to taka postawa nigdzie nie prowadzi. Dodatkowo, powszechnie wiadomo, że głupiec mądrego też będzie miał za głupca, bo tak działają nasze mózgi, że bierzemy nasze interpretacje jako prawdy objawione (przynajmniej w pierwszej chwili), choć zwykle są bardzo luźno związane z rzeczywistością. Tym bardziej, że wiem jak to jest wkurzyć się na kolegę w pracy, ale bez przesady, w wielu przypadkach zaczynam rozumieć ich punkt widzenia lub dochodzę do tego, że to jednak ja dałem ciała. Nigdy nie jest się nieomylnym w 100% przypadków. To jest technicznie niemożliwe z uwagi na to, że nasze zmysły są ograniczone i nasze decyzje podejmujemy na podstawie niepełnych danych.

proximus-prime napisał(a):

Kod bardzo wiele mowi o jego tworcy i jeslI jest napisany we wlasciwy sposob pozwala juniorowi szybko sie wdrozyc w projekt, a rownoczesnie duzo nauczyc w ograniczonym zakresie czasu.

Ale tylko wtedy, gdy junior się nadaje do programowania. Są tacy, którzy nic nie potrafią i nie przejawiają żadnej zdolności do niczego poza pisaniem gównokodu spaghetti.
Niestety, pisać kod szybko i byle jak jest dość łatwo się nauczyć. Ale pisać dobry kod już trudniej. Niektórzy na zawsze zatrzymują się na tym pierwszym, a gdy ktoś chce wymusić dobre praktyki reagują mniej więcej tak jak autor tego wątku.

Poza tym, dlatego junior nie jest seniorem, że może nie zdawać sobie sprawy, że czasami nawet warto poświęcić więcej czasu na dobrą implementację po to, żeby potem po czasie nie okazało się że wszystko co powstało to g**no i w sumie to trzeba by zacząć to od początku inaczej.

Jeśli ktoś nic nie umiejąc ma takie podejście jak tu widać to nie nadaje się do zespołu.

Garen_eye napisał(a):

... Najgorsze co można zrobić chyba, to zostawić proponowany kod w Changes Requested bez komentarza PO CO i DLACZEGO TAK.

To jest bardzo wazne o czym piszesz, bo wnosi dla pracy juniora nieoceniona wartosc merytoryczna. Wiem z wlasnego doswiaczenia, ze tego rodzaju podejscie przynosi doskonale rezultaty, zarowno w sesnie edukacyjnym jak i mentalnym. Chociaz w CR'kach zdarzaja sie takie kwiatki, jak pojedyncze pytania: A czemu tak? albo Tak sie nie robi, do poprawki junior baranieje i nikt z niego nie ma zadnego pozytku.

Jak zwykle, diabeł tkwi w szczegółach. Jeśli już 10 razy junior zrobił tak tak się nie robi i tłumaczono mu, że tak się nie robi, a za 11 zrobił znów to samo, to komentarz Tak sie nie robi, do poprawki już powinien wystarczyć.

gajusz800 napisał(a):

Ale tylko wtedy, gdy junior się nadaje do programowania. Są tacy, którzy nic nie potrafią i nie przejawiają żadnej zdolności do niczego poza pisaniem gównokodu spaghetti.

Wybacz, ale kim Ty jestes zeby to oceniac lub o tym decydowac? Powiem Ci taka rzecz. Mialem niedawno do czynienia z juniorem, ktorego sam rekrutowalem i sam podjalem decyzje o jego zaangazowaniu.
Mial ta iskre w oku, kiedy opowiadal o swoim kodzie w repo, o swoich zainteresowaniach itp itd.

Po miesiacu okazalo sie jednak, ze gosc za cholere nie potrafi wejsc w projekt. Dziesiec razy omawialem z nim architekture, tllumaczylem powiazania, wyjasnialem filozofie kodu, a on k...wa, za kazdym razem wszystko robil inaczej niz oczekiwalem. Pozegnalbym sie z nim uznajac swoje umiejetnosci oceny personalnej jako rowne zero, gdyby nie jedna mala rzecz, przypomnialem sobie ze gosc w trakcie rozmowy technicznej caly czase operowal olowkiem i kartka papieru rownoczesnie tlumaczac mi calkiem zawile kwestie. Kiedy zdalem sobie z tego sprawe, sprobowalem rysowac wszystko to co do tej pory przekazywalem mu jedynie werbalnie, a teraz gosc po prostu WYMIATA po calosci.

Z'puentuje to wiec w ten sposob. Ostroznie z powierzchownymi obserwacjami i pochopnymi wnioskami.

Już pisałem, że nie wiem czy tak jest w tym konkretnym przypadku, ale z tego co widziałem to najczęściej tak właśnie jest. A o takiej możliwości nikt tu nie wspomniał - więc ktoś musi.

Może za późno dajesz rzeczy do review, możesz przecież dać do sprawdzenia coś, co jest "work in progress", wtedy na wcześniejszym etapie prac możesz dostać feedback.

No i zanim zaczniesz robić, też możesz przegadać podejście.

Plus zobacz na to, jak już jest projekt napisany, czy na pewno stosujesz się do standardów w projekcie.

Próbowałem mówić mu że to będzie za duża zmiana i jeśli mamy działające moim sposobem, to po co mam kolejny dzień spędzać na przebudowaniu tego samego. Jego odpowiedz: nie narzekaj, to mala zmiana, musimy teraz dodac. Przez takie akcje nie chce mi sie tam pracowac.

Ogólnie jakaś taka toksyczna symbioza jest między wami.
On chce, żeby wszystko było tak, jak sobie wymyślił, i chce cię do tego zmusić, bo z jakichś powodów uważa, że tylko on ma rację.
Ty nie masz odwagi się mu postawić, ale czujesz jak rozumiem frustrację i bunt, oraz niepewność, czy zdążysz dowieźć sprint?

Być może do zespołów programistycznych powinno też się rekrutować psychologa, żeby było gdzie rozwiązywać tego rodzaju konflikty.

proximus-prime napisał(a):

Calkiem mozliwe, ze w universum Marvela, w ktorym Wy funkcjonujecie i w ktorym nikogo nie dziwi szop pracz biegajacy z bazuka na pokladzie statku kosmicznego to o czym piszecie jest czyms normalnym natomiast tu na Ziemi gdzie toczy sie normalne zycie sprawy wygladaja zgola inaczej.

Ja po prostu nie pracuję na plantacji spaghetti, i nie mam zadań w rodzaju "napisz 500 linijek kodu", więc nikt ode mnie tego nie wymaga.

Strasznie autorytatywnie się wypowiadasz jak na kogoś, kto wybitnie mało w życiu zawodowym widział.

somekind napisał(a):

Strasznie autorytatywnie się wypowiadasz jak na kogoś, kto wybitnie mało w życiu zawodowym widział.

O moim zyciu zawodowym wiesz tyle co przeczytasz na tym forum (de facto null), a swojego nie masz bo koczujesz tu 24/24 zajmujac sie wylacznie banowaniem niezgodnych ideowo z twoimi pogladow.
Po co zaczynasz pyskowke?

@adamKowal
" Bez zastanowienia wpieprza swoje uwagi czy potrzebne czy nie aby Jego nie obchodzi to że feature opóźni się i kto później będzie się z tego spowiadał?" - "Talk is cheap show me the code" ;) Jeśli nie możesz pokazać kodu przed i po review to czy jesteś pewny, że uwagi opóźniły feature a nie powstrzymały buga na produkcji? Albo oszczędziły godziny debugowania w czasie incydentu? Jesteś w stanie pokazać np. o ile opóźnienie feature zmniejszy zysk firmy, zadowolenie klientów?

"Jego uwagi i wyssane z d**y kryteria" - zwykle to uwagi są wyssane z doświadczenia.

"A na pytania żeby rozwinął o co mu chodzi konkretnie w tej zmianie dlaczego uważa że jego sposobem jest lepiej - nie dyskutuj." - "Change the organisation or change the organisation"

"Nie wspominając już że psychicznie obciążające jest wymyślanie koła na nowo pod wymagania." - jak ktoś w review sugeruje, że trzeba coś zmienić, to z wymyśłeniem na nowo też powinien pomóc.

"W każdej zmianie wieszcza czarne scenariusze, dlatego że nie będzie zmienione za jakiś czas projekt sie rozjebie. Bardzo mnie to śmieszy." - 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.