Aktualnie pracuję w dość małym teamie z seniorami będąc juniorem (~2 lata doświadczenia w obecnej technologii), ten układ pozwala mi wyciągnąć wiele wiedzy z code review dokonanego przez moich kolegów. Co jest dość oczywiste, oni też dodają mnie do swoich pull requestów, więc w ten sposób też mogę się czegoś nauczyć, ba, nawet może znaleźć jakieś niedociągłości w ich kodzie. Dlatego teraz pojawiło się pytanie z mojej strony - jak podchodzić do code review kolegów seniorów (będąc formalnie juniorem) tak, aby wyciągnąć z tego jak najwięcej i żeby dodać ewentualnie coś od siebie? Jak wy do tego podchodzicie? Analizujecie pull request w IDE? Jak staracie się zrozumieć kontekst bazując na kodzie - sprawdzacie ticket, a potem dokładnie analizujecie kod? Ile czasu zajmuje wam robienie komuś code review? W jaki sposób myśleć o analizowanym kodzie tak, aby na przykład wykryć potencjalne bugi?
Z mojego punktu widzenia najwięcej zapamiętuje jeżeli jest to mi wytknięte w moim kodzie, z kolei mam problem z stosowaniem praktyk zaobserwowanych u kolegów w ich PR. Przypuszczam, że po prostu źle podchodzę do robienia tego review, dlatego zapytuję.
Zależy od wielkości. Jeśli to jakieś małe zmiany to tylko przez stronę patrze. Jak widzę jakiegoś większego PRa i akurat nie mam co robić pilnego to ściągam sobie brancha i chodzę przez IDE (łatwiejsza nawigacja). Jeśli są, to najlepiej zacząć od testów bo pokazują mniej więcej o co chodzi.
Jeżeli chcesz wyciągnąć z tego jak najwiecej to warto zastanowić się dlaczego ktoś napisał ten kod w taki, a nie inny sposób. Ewentualnie zawsze możesz dopytać.
Zależy od wielkości i ważnosci PRa, male rzeczy to ogarniam na stashu/gerricie/gicie czymkolwiek. No i też zależy komu. Porządny PR to zcheckoutwanie kodu u siebie, najpierw sprawdzam w testach co dany kod ma robić, potem patrzę na architekturę, nastepnie implementacja. Jeżeli chodzi o testy to najwazniejsza jest dla mnie czytelność testów, staram się też wymyśleć inne przypadki niż autor i podstawić do jego kodu i sprawdzić czy przejdzie. Przy okazji uruchamiam kod lokalnie lub na środowisku developerskim i sprawdzam jak się korzysta z danej funkcjonalności - czy wszystko jest oczywiste i czy nie można niczego poprawić z perspektywy UXowej. Tak samo gdy ktoś wrzuca kod infrastrukturalny/libkę - czy jako potencjalny użytkownik API wszystko rozumiem i nic nie jest zbyt skomplikowane lub nie wycieka logika. Zjada to sporo czasu, wiec czasem omijam niektóre punkty, tez bardziej się przykładam do review mniej doświadczonych osób żeby jak najwięcej z tego wyciągnęły, No i IMHO też wazne jest zadawanie pytan, jeżeli czegos nie rozumiesz w kodzie to warto o to zapytać, bo albo się czegoś dowiesz albo coś można poprawić żeby było chociażby bardziej zrozumiałe.
Jest prosta zasada:
Jak jest mały PR to spokojnie się w web na git ogląda.
Jak jest duży to po prostu należy odrzucić :-)
Niestety:
W praktyce nie udało mi się tej reguły stosować, wymusić na dłużej, ale widziałem, że jest dość zdrowa. Znam teamy, które coś takiego mają.
Wiekszość PR jest niepotrzebnie długa. Może być rozbita na analizowalne kawałki. (a znana zasada mówi: jak zrobisz commit na 10 linijek to ludzie znajdą tam 5 problemów. jak zrobisz na 10000 to nie znajdą żadnego)
Na tickety nie patrzę, bo po pierwsze wiem kto nad czym pracuje, po drugie nasz BA jest tak bezużyteczny, że w tickecie poza summary i tak niczego nie ma.
Jeśli chodzi o PR, to zależy od tego, kto implementował. Po ogarniętych ludziach nie ma się co spodziewać ciekawych fakapów, więc wszystkie można wypatrzeć w web gicie.
Jeśli zaś chodzi o kod cieniasów, to są dwa podejścia - po pierwsze bardzo dużo bzdur widać w przeglądarce (typu if, który jest zawsze false, pętla, która przechodzi tylko raz, brak pustych liniej kodu tam gdzie trzeba, a ich nadmiar gdzie nie trzeba, zwalone formatowanie, brak testów, wynajdowanie koła na nowo). Ale czasami widzi się taki fakap, którego bez pobrania i odpalenia kodu się nie ogarnie - np. jeśli kod jest zepsuty w kilku miejscach tak, że błędy się niwelują i potrafi zadziałać w 95% przypadków.
rieder napisał(a):
jak podchodzić do code review kolegów seniorów (będąc formalnie juniorem) tak, aby wyciągnąć z tego jak najwięcej i żeby dodać ewentualnie coś od siebie?
Nie skupiaj się na nieistotnych rzeczach typu formatowanie, linijka pusta, a skup na tym co istotne. Oczywiście możesz o tym wspomnieć, zwłaszcza jeśli np to literówka, ale skup się na architekturze i czytelności kodu. Czy ty go rozumiesz? Tak - ok, dlaczego? Czy to ze względu na prostotę rzeczy do zrobienia, czy użyte wzorce?
Nie - ok, dlaczego? Nie rozumiesz zasady działania systemu albo frameworka lub wzorca? Dobra okazja żeby się nauczyć.
Do tego popatrz na testy i zastanów się czy są czytelne i czy mają wystarczające pokrycie. I czy nie testują implementacji.
Zwróć uwagę też jak inni oceniają kod i jakie mają uwagi do niego. Poczytaj komentarze innych osób między sobą.