Co zrobić, gdy kod przesłany do przeglądu kodu wydaje się zbyt skomplikowany?

115

Kod jest trudny do naśladowania, ale wydaje się, że (przeważnie) działa dobrze, przynajmniej w przypadku powierzchownych testów. Mogą występować małe błędy tu i tam, ale bardzo trudno jest stwierdzić, czytając kod, czy są symptomy głębszych problemów lub prostych poprawek. Ręczna weryfikacja ogólnej poprawności za pomocą przeglądu kodu jest jednak bardzo trudna i czasochłonna, jeśli w ogóle jest to możliwe.

Jaki jest najlepszy sposób działania w tej sytuacji? Domagać się remontu? Częściowe wycofanie? Najpierw faktoring? Napraw tylko błędy i zaakceptuj dług techniczny ? Dokonaj oceny ryzyka tych opcji, a następnie zdecyduj? Coś innego?

Brad Thomas
źródło
4
Pojawia się czy jest? Szybki pomiar plików źródłowych potwierdzi lub odrzuci twoje podejrzenia. Po pomiarze po prostu wyświetl numery pomiarowe podczas przeglądu kodu i zasugeruj refaktoryzację, aby obniżyć liczby złożoności.
Jon Raynor,
4
Proszę zdefiniować „zbyt skomplikowane”. Czy kod jest zbyt skomplikowany, ponieważ wykorzystuje dobrze znane wzorce projektowe, które są po prostu nieznane Twojemu zespołowi, lub ponieważ nie używa wzorców dobrze znanych Twojemu zespołowi? Dokładne powody, dla których kod jest oceniany jako „zbyt skomplikowany”, są niezbędne do prawidłowej oceny dalszego postępowania. Oświadczenie tak proste, jak „zbyt skomplikowane” w obszarze wiedzy tak głębokim i skomplikowanym, jak przegląd kodu, sugeruje mi polowanie na programistę.
Pieter Geerkens,
7
@PieterGeerkens A może jest to zbyt skomplikowane, ponieważ rozwiązuje skomplikowany problem?
Casey

Odpowiedzi:

251

Jeśli nie można go przejrzeć, nie może przejść przeglądu.

Musisz zrozumieć, że przegląd kodu nie służy do znajdowania błędów. Do tego służy kontrola jakości. Przegląd kodu ma na celu zapewnienie możliwości przyszłej konserwacji kodu. Jeśli nie możesz teraz postępować zgodnie z tym kodem, jak możesz to zrobić w ciągu sześciu miesięcy, kiedy zostaniesz przydzielony do ulepszeń funkcji i / lub poprawek błędów? Znalezienie błędów w tej chwili to tylko dodatkowa korzyść.

Jeśli jest zbyt skomplikowany, narusza masę zasad SOLID . Refaktor, refaktor, refaktor. Podziel je na odpowiednio nazwane funkcje, które działają o wiele mniej, prościej. Możesz to wyczyścić, a twoje przypadki testowe upewnią się, że nadal działa poprawnie. Masz przypadki testowe, prawda? Jeśli nie, zacznij je dodawać.

Opłata Kevin
źródło
49
To bardzo. Pamiętaj, że nie tylko ty i pisarz będziecie czytać ten kod. Będzie to także losowy stażysta za 10 lat, więc chcesz mieć pewność, że ma szansę zrozumieć, co się dzieje.
David Grinberg,
2
dobra odpowiedź. zależy to od celu „przeglądu kodu”. czytelność to jedna rzecz, struktura inna - ale bardzo ściśle ze sobą powiązane. fwiw Pracuję z jakimś open source napisanym przez MAJOR corps, i jest to prawie nieczytelne, ponieważ nazwy var i fn są tak mózgowe.
19
@DavidGrinberg Ze wszystkich praktycznych powodów „ty za sześć miesięcy” to zupełnie inna osoba.
Chrylis -on strike-
2
Odłóż kod na jakiś czas (wystarczająco długo, aby nie pamiętał wszystkiego). Poproś oryginalnego programistę o sprawdzenie. Sprawdź, czy ON to rozumie.
Nelson
4
Nie zgadzam się, że recenzja kodu „nie” służy do znajdowania błędów. Często znajduje błędy i jest to bardzo potężny i użyteczny aspekt recenzji kodu. Co więcej, pomaga znaleźć sposoby na całkowite uniknięcie błędów w przyszłym kodzie. Chodzi o to, że jest to przesadzone i powinno być tak, że nie chodzi tylko o znajdowanie błędów!
Cody Gray,
45

Wszystko, o czym wspominasz, jest w pełni poprawne do wskazania w przeglądzie kodu.

Po otrzymaniu recenzji kodu sprawdzam testy. Jeśli testy nie zapewniają wystarczającego zasięgu, należy na to zwrócić uwagę. Testy muszą być przydatne, aby upewnić się, że kod działa zgodnie z przeznaczeniem i będzie działał zgodnie z przeznaczeniem w przypadku zmian. W rzeczywistości jest to jedna z pierwszych rzeczy, których szukam w recenzji kodu. Jeśli nie udowodniłeś, że Twój kod spełnia wymagania, nie chcę poświęcać czasu na jego przeglądanie.

Gdy są wystarczające testy kodu, jeśli kod jest złożony lub trudny do naśladowania, ludzie powinni na to spojrzeć. Narzędzia analizy statycznej mogą wskazać pewne miary złożoności i oznaczyć zbyt złożone metody, a także znaleźć potencjalne wady w kodzie (i powinny zostać uruchomione przed przeglądem kodu ludzkiego). Ale kod jest odczytywany i obsługiwany przez ludzi i musi być napisany w celu zachowania łatwości obsługi. Tylko jeśli istnieje powód, aby używać mniej konserwowalnego kodu, powinien być napisany w ten sposób. Jeśli potrzebujesz kodu złożonego lub nieintuicyjnego, powinieneś udokumentować (najlepiej w kodzie), dlaczego kod jest w ten sposób i mieć pomocne komentarze dla przyszłych programistów, aby zrozumieć, dlaczego i co robi kod.

Najlepiej odrzuć recenzje kodu, które nie mają odpowiednich testów lub mają zbyt skomplikowany kod bez uzasadnionego powodu. Mogą istnieć powody biznesowe, aby iść naprzód, a do tego musisz ocenić ryzyko. Jeśli nadal będziesz mieć dług techniczny w kodzie, natychmiast umieść bilety w systemie śledzenia błędów, podając szczegółowe informacje o tym, co należy zmienić, oraz sugestie dotyczące jego zmiany.

Thomas Owens
źródło
30

Ręczna weryfikacja ogólnej poprawności za pomocą przeglądu kodu jest jednak bardzo trudna i czasochłonna, jeśli w ogóle jest to możliwe.

To nie jest sens przeglądu kodu. Sposób myślenia o recenzji kodu polega na wyobrażeniu sobie, że w kodzie jest błąd i trzeba go naprawić. W tym sposobie myślenia przejrzyj kod (szczególnie komentarze) i zadaj sobie pytanie: „Czy łatwo jest zrozumieć duży obraz tego, co się dzieje, aby zawęzić problem?” Jeśli tak, to przepustka. W przeciwnym razie jest to błąd. Potrzebna jest co najmniej większa dokumentacja lub ewentualnie refaktoryzacja, aby kod był zrozumiały.

Ważne jest, aby nie być perfekcjonistą, chyba że masz pewność, że tego właśnie szuka twój pracodawca. Większość kodu jest do kitu tak bardzo, że można go łatwo refaktoryzować 10 razy z rzędu, za każdym razem zapewniając większą czytelność. Ale twój pracodawca prawdopodobnie nie chce płacić za najbardziej czytelny kod na świecie.

DepressedDaniel
źródło
4
Świetny komentarz! „Większość kodu jest do kitu tak bardzo, że można go łatwo zrewidować 10 razy z rzędu, za każdym razem zyskując większą czytelność” Chłopcze, byłem tego winien :)
Dean Radcliffe,
1
„Większość kodu jest do kitu tak bardzo, że można go z łatwością ponownie przetworzyć 10 razy z rzędu, za każdym razem zapewniając większą czytelność”. Rzeczywiście tak jest w prawdziwym świecie.
Peter Mortensen
@PeterMortensen Rzeczywiście prawdą jest, że dużo tego można znaleźć w prawdziwym świecie. Ale pisanie kodu w ten sposób nie leży w niczyim interesie. Myślę, że istnieją dwa powody, dla których tak jest. Edukacja, którą otrzymują programiści, wkłada bardzo mało wysiłku w nauczanie pisania czytelnego kodu. A w niektórych firmach jest to strata czasu: „Jeśli programista napisał już działający kod, dlaczego miałoby nas obchodzić, czy jest on czytelny, czy nie?
kasperd,
15

Ręczna weryfikacja ogólnej poprawności za pomocą przeglądu kodu jest jednak bardzo trudna i czasochłonna, jeśli w ogóle jest to możliwe.

Wiele lat temu właściwie moim zadaniem było robić dokładnie to, oceniając zadania domowe uczniów. I podczas gdy wielu dostarczyło rozsądną jakość z błędem tu i tam, było dwóch, którzy się wyróżnili. Oba zawsze przesyłały kod, który nie zawierał błędów. Jeden przesłany kod, który mogłem odczytać z góry i dołu z dużą prędkością i oznaczyć jako 100% poprawny przy zerowym wysiłku. Drugi przesłał kod, który był jednym WTF po drugim, ale jakoś udało się uniknąć błędów. Absolutny ból do zaznaczenia.

Dzisiaj drugi z nich miałby odrzucony kod podczas przeglądu kodu. Jeśli weryfikacja poprawności jest bardzo trudna i czasochłonna, jest to problem z kodem. Przyzwoity programista wymyśliłby, jak rozwiązać problem (wymaga czasu X), a przed przekazaniem go do przeglądu kodu refaktoryzuje go tak, aby nie tylko wykonał zadanie, ale oczywiście wykonał zadanie. To zajmuje znacznie mniej niż X czasu i pozwala zaoszczędzić dużo czasu w przyszłości. Często odkrywając błędy, zanim jeszcze przejdą do etapu przeglądu kodu. Następnie sprawiając, że przegląd kodu jest znacznie szybszy. I cały czas w przyszłości, ułatwiając dostosowanie kodu.

Inna odpowiedź głosiła, że ​​kod niektórych osób może być refaktoryzowany 10 razy, za każdym razem staje się bardziej czytelny. To po prostu smutne. To programista, który powinien poszukać innej pracy.

gnasher729
źródło
10 razy zajmuje mi mniej czasu refaktoryzacja kodu, a następnie napisanie pierwszej wersji kodu. Jeśli ktokolwiek wie, że dokonałem tego refaktoryzacji, nie udało mi się.
Ian
6

Czy ten stary kod został nieco zmieniony? (100 linii kodu zmienionych w bazie kodu 10000 linii to wciąż niewielka zmiana) Czasami istnieją ograniczenia czasowe i programiści muszą pozostać w starej i niewygodnej strukturze, po prostu dlatego, że całkowite przepisanie potrwa jeszcze dłużej i jest znacznie poniżej budżetu . + zwykle wiąże się to z ryzykiem, które może niepotrzebnie kosztować miliony dolarów. Jeśli jest to stary kod, w większości przypadków będziesz musiał z nim żyć. Jeśli nie rozumiesz tego sam, porozmawiaj z nimi i słuchaj ich wypowiedzi, spróbuj je zrozumieć. Pamiętaj, że może być dla ciebie trudny do naśladowania, ale dla innych osób jest w porządku. Po ich stronie, zobacz to od końca.

Czy to nowy kod ? W zależności od ograniczeń czasowych powinieneś zalecać refaktoryzację w jak największym stopniu. Czy w razie potrzeby można poświęcić więcej czasu na recenzje kodu? Nie powinieneś sam mierzyć czasu do 15 minut, wpadnij na pomysł i idź dalej. Jeśli autor poświęcił tydzień na napisanie czegoś, dobrze jest poświęcić 4-8 godzin na jego sprawdzenie. Twoim celem tutaj jest pomoc w refaktoryzacji. Nie zwracasz tylko kodu „refaktoryzuj. Teraz”. Sprawdź, które metody można rozbić, spróbuj wymyślić pomysły na wprowadzenie nowych zajęć itp.

Neolisk
źródło
2
Nie zwracasz tylko kodu „refaktoryzuj. Teraz” - dlaczego? Otrzymałem takie komentarze co najmniej raz i ostatni raz pamiętam, że okazało się pomocne i poprawne. Musiałem przepisać dużą część kodu od zera i było to słuszne, ponieważ patrząc wstecz widziałem, że stary kod był nie do uratowania. Recenzent był po prostu wystarczająco wykwalifikowany, aby to zauważyć (i najwyraźniej nie byłem)
komnata
4
@gnat: Po pierwsze, bo to niegrzeczne. Wyglądasz lepiej, gdy wyjaśnisz, co jest nie tak z kodem i podejmiesz wysiłek, aby pomóc drugiej osobie go poprawić. W dużej firmie może to doprowadzić do szybkiego wyjścia z domu. Zwłaszcza jeśli przeglądasz kod osoby starszej.
Neolisk
że przypadek I, o których mowa powyżej, było w dużej siedzibę firmy, które po prostu okazał się być dość ostrożnym, nie wychodzi z drzwi swoich najbardziej wykwalifikowanych programistów, a przynajmniej nie na podstawie bezpośredniego dzielenia się swoją wiedzą techniczną, gdy poproszony o
komara
1
@gnat: Podejście „refactor. now” może działać w dół, tj. gdy starszy programista z ponad 10-letnim doświadczeniem mówi o refaktoryzacji do młodszego programisty, który został zatrudniony 1 miesiąc temu bez doświadczenia lub podobnej sytuacji. W górę - możesz mieć problem. Ponieważ możesz nie wiedzieć, ile doświadczenia ma drugi programista, można bezpiecznie przyjąć, że jest to zachowanie domyślne. Na pewno cię to nie skrzywdzi.
Neolisk
1
@ Neolisk: Doświadczony programista, który musiał pisać kod pod presją czasu i wie, że nie jest wystarczająco dobry, może być zbyt szczęśliwy, jeśli odrzucisz kod, dając mu czas i pretekst do jego ulepszenia. PHB decydując, że jest wystarczająco dobry, czyni dewelopera nieszczęśliwym; recenzent decydujący, że nie jest wystarczająco dobry, czyni go szczęśliwym.
gnasher729,
2

Często „skomplikowane” łatki / listy zmian to takie, które wykonują wiele różnych rzeczy naraz. Jest nowy kod, usunięty kod, kod refaktoryzowany, przeniesiony kod, rozszerzone testy; utrudnia to dostrzeżenie dużego obrazu.

Powszechną wskazówką jest to, że łatka jest ogromna, ale jej opis jest niewielki: „Implementuj $ FOO”.

Rozsądnym sposobem radzenia sobie z taką łatką jest poproszenie jej o podzielenie jej na serię mniejszych, samodzielnych elementów. Tak jak zasada pojedynczej odpowiedzialności mówi, że funkcja powinna robić tylko jedną rzecz, tak łatka powinna koncentrować się również na jednej rzeczy.

Na przykład pierwsze łatki mogą zawierać czysto mechaniczne refaktoryzacje, które nie wprowadzają żadnych zmian funkcjonalnych, a następnie ostatnie łatki mogą skupić się na faktycznej implementacji i testowaniu $ FOO przy mniejszej liczbie rozproszeń i czerwonych śledzi.

W przypadku funkcji wymagającej dużej ilości nowego kodu nowy kod można często wprowadzić w testowalnych porcjach, które nie zmieniają zachowania produktu, dopóki ostatnia łatka z serii nie wywoła nowego kodu (odwrócenie flagi).

Jeśli chodzi o robienie tego taktownie, zwykle wypowiadam to jako swój problem, a następnie proszę o pomoc autora: „Mam problem ze śledzeniem wszystkiego, co się tutaj dzieje. Czy możesz podzielić tę łatkę na mniejsze kroki, aby pomóc mi zrozumieć, jak to wszystko pasuje razem?" Czasami konieczne jest przedstawienie konkretnych sugestii dotyczących mniejszych kroków.

Tak duża łata, jak „Implement $ FOO”, zamienia się w serię łatek takich jak:

  1. Przedstaw nową wersję Frobnicate, która wymaga pary iteratorów, ponieważ będę musiał wywołać ją z sekwencjami innymi niż wektor, aby zaimplementować $ FOO.
  2. Przełącz wszystkich istniejących rozmówców Frobnicate, aby użyć nowej wersji.
  3. Usuń stary Frobnicate.
  4. Frobnicate robił za dużo. Uwzględnij krok przeróbki we własnej metodzie i dodaj do tego testy.
  5. Przedstaw Zerzify wraz z testami. Jeszcze nie używany, ale będę go potrzebować do $ FOO.
  6. Zaimplementuj $ FOO w kategoriach Zerzify i nowego Frobnicate.

Należy pamiętać, że kroki 1-5 nie wprowadzają żadnych zmian funkcjonalnych w produkcie. Są trywialne do sprawdzenia, w tym upewnienia się, że masz wszystkie odpowiednie testy. Nawet jeśli krok 6 jest nadal „skomplikowany”, przynajmniej koncentruje się na $ FOO. Dziennik w naturalny sposób daje o wiele lepsze wyobrażenie o tym, w jaki sposób wdrożono $ FOO (i dlaczego zmieniono Frobnicate).

Adrian McCarthy
źródło
Jednym podejściem, jeśli używasz Git, jest skomponowanie żądania ściągania wielu zatwierdzeń. Każde zatwierdzenie jest tak atomowe i niezależne, jak to możliwe, i ma swój własny opis. Następnie dodaj przydatną treść w treści PR, że każdą zmianę można przejrzeć ręcznie. Zasadniczo tak radzę sobie z bardzo dużymi PR-ami, takimi jak globalne refaktory lub duże, niedostępne zmiany w narzędziach.
Jimmy Breck-McKye
1

Jak zauważyli inni, przegląd kodu nie jest tak naprawdę przeznaczony do znajdowania błędów. Jeśli podczas sprawdzania kodu znajdziesz błędy, prawdopodobnie oznacza to, że nie masz wystarczającej liczby automatycznych testów (np. Testów jednostkowych / integracyjnych). Jeśli nie ma wystarczającego zasięgu, aby przekonać mnie, że kod robi to, co powinien , zwykle proszę o więcej testów i wskazuję rodzaj przypadków testowych, których szukam, i zwykle nie wpuszczam kodu do bazy kodu, która nie ma odpowiedniego zasięgu .

Jeśli architektura wysokiego poziomu jest zbyt złożona lub nie ma sensu, zwykle zwołuję spotkanie z kilkoma członkami zespołu, aby o tym porozmawiać. Czasami trudno jest powtarzać złą architekturę. Jeśli deweloper był nowicjuszem, zwykle upewniam się, że przejdziemy to, co ich myślenie wyprzedza, zamiast reagować na złe żądanie ściągnięcia. Jest to zwykle prawdą nawet w przypadku bardziej doświadczonych programistów, jeśli problem nie ma oczywistego rozwiązania, które można by bardziej niż prawdopodobnie wybrać.

Jeśli złożoność jest izolowana do poziomu metody, który zwykle można poprawić iteracyjnie i za pomocą dobrych testów automatycznych.

Ostatni punkt. Jako recenzent musisz zdecydować, czy złożoność kodu wynika z niezbędnej, czy przypadkowej złożoności . Podstawowa złożoność dotyczy części oprogramowania, które są prawnie trudne do rozwiązania. Przypadkowa złożoność odnosi się do wszystkich pozostałych części kodu, który piszemy, który jest zbyt złożony bez żadnego powodu i można go łatwo uprościć.

Zazwyczaj upewniam się, że kod o zasadniczej złożoności jest naprawdę taki i nie można go dalej upraszczać. Dążę również do zwiększenia zasięgu testów i dobrej dokumentacji dla tych części. Przypadkowa złożoność powinna być prawie zawsze usuwana podczas procesu żądania ściągania, ponieważ stanowią one większość kodu, z którym mamy do czynienia, i mogą łatwo spowodować koszmar konserwacji, nawet w krótkim okresie.

c_maker
źródło
0

Jakie są testy? Powinny być jasne, proste i łatwe do odczytania, najlepiej tylko z jednym stwierdzeniem. Testy powinny jasno dokumentować zamierzone zachowanie i przypadki użycia kodu.

Jeśli nie jest to dobrze przetestowane, jest to dobre miejsce do rozpoczęcia recenzji.

Tom Squires
źródło