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?
code-reviews
Brad Thomas
źródło
źródło
Odpowiedzi:
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ć.
źródło
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.
źródło
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.
źródło
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.
źródło
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.
źródło
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:
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).
źródło
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.
źródło
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.
źródło