Co powinienem zrobić jako recenzent, gdy kod „musi być” zły?

Pracuję w źle zaprojektowanym, nadinżynieryjnym projekcie, w którym kilka miesięcy temu wprowadzono obowiązkowe przeglądy kodu.

Poproszono mnie o zapoznanie się z dużym fragmentem kodu, który implementuje nową funkcję. Ma te same wady, co reszta naszej bazy kodów. Rozumiem, że w dużym stopniu te wady wkradają się do nowego kodu, np. poprzez konieczność dziedziczenia po źle zaprojektowanej klasie lub implementacji źle zaprojektowanego interfejsu przy jednoczesnym zachowaniu kompatybilności, a tak naprawdę nie mogę zaoferować znacznie lepszego rozwiązania, które nie wymagałoby przepisywania połowy bazy kodu. Ale czuję, że z inżynieryjnego punktu widzenia nie ma już nic dobrego dla już złamanej bazy kodu, że łamiemy ją jeszcze bardziej. Kod, który recenzuję, jest zdecydowanie zły, ale musi być, jeśli funkcja ma zostać zaimplementowana.

Jak mam się zachowywać w odniesieniu do tej konkretnej recenzji? Czy jest jakiś sposób, aby zachować uczciwość i zachować konstruktywność?

Pamiętaj, że w tym kontekście pytam o granicę przeglądu kodu. Zdaję sobie sprawę, że problem jest spowodowany innymi czynnikami, w tym organizacją i kulturą pracy, ale chcę wiedzieć, jak sobie z tym poradzić.

Prosty:

1. Dokumentuj swój dług techniczny

Zidentyfikowałeś fragment kodu, który działa, ale masz z nim pewne problemy techniczne. To dług techniczny . Podobnie jak inne rodzaje długów, z czasem pogarsza się, jeśli nie zostanie uregulowane.

Pierwszym krokiem w radzeniu sobie z długiem technicznym jest jego udokumentowanie. Zrób to, dodając elementy do śledzenia problemów, które opisują dług. Pomoże to uzyskać wyraźniejszy obraz skali problemu, a także pomoże w opracowaniu planu rozwiązania tego problemu.

2. Stopniowo spłacaj swój dług

Zmodyfikuj proces tworzenia oprogramowania, aby uwzględnić spłatę zadłużenia technicznego. Może to obejmować sporadyczny sprint twardnienia lub po prostu rozwiązywanie długów w regularnych odstępach czasu (np. 2 przedmioty tygodniowo). Ważna część, aby upewnić się, że spłacasz swój dług szybciej niż narasta (dług, nawet techniczny, ma na nim odsetki).

Po osiągnięciu punktu, w którym nie masz już deficytu technicznego , jesteś w drodze do zdrowszej bazy kodów :)

Na marginesie: poszukaj nowej pracy. Ten nie byłby lepszy.

Cele sprawdzanego kodu to:

• Aby wysłać funkcję, która powinna działać zgodnie z wymaganiami.

• Ograniczenie wzrostu zadłużenia technicznego.

Pierwszy cel jest weryfikowany przez sprawdzenie, czy testy jednostkowe, integracyjne, systemowe i funkcjonalne są tutaj, czy są odpowiednie i czy obejmują wszystkie sytuacje, które muszą zostać przetestowane. Musisz także sprawdzić przekonania autora na temat języka programowania, co może prowadzić do subtelnych błędów lub kodu udającego, że robi coś innego niż to, co faktycznie robi.

Drugi cel to ten, na którym koncentruje się twoje pytanie. Z jednej strony nie oczekuje się, że nowy kod zwiększy zadłużenie techniczne. Z drugiej strony zakresem przeglądu jest sam kod, ale w kontekście całej bazy kodu. Odtąd jako recenzent możesz oczekiwać dwóch podejść od oryginalnego autora:

• Kod zewnętrzny nie jest moją winą. Po prostu implementuję tę funkcję i nie dbam o całą bazę kodu.

  W tej perspektywie kod skopiuje wady bazy kodu, a zatem nieuchronnie zwiększy techniczny dług: więcej złego kodu jest zawsze gorsze.

  Chociaż jest to prawidłowe podejście krótkoterminowe, w perspektywie długoterminowej spowodowałoby to zwiększenie opóźnień i niską wydajność, a ostatecznie doprowadziłoby do tak kosztownego i ryzykownego procesu rozwoju, że produkt przestałby ewoluować.

• Napisanie nowego kodu jest okazją do refaktoryzacji starszego kodu.

  W tej perspektywie wpływ wad starego kodu na nowy może być ograniczony. Co więcej, dług techniczny można zmniejszyć lub przynajmniej nie zwiększyć proporcjonalnie do wzrostu kodu.

  Chociaż jest to prawidłowe podejście długoterminowe, wiąże się z nim ryzyko krótkoterminowe. Głównym z nich jest, że krótkotrwałe, to czasami wymaga więcej czasu, aby wysyłać funkcji konkretnego. Innym ważnym aspektem jest to, że jeśli starszy kod nie zostanie przetestowany, jego refaktoryzacja stwarza ogromne ryzyko wprowadzenia regresji.

W zależności od perspektywy, którą chcesz zachęcić, możesz być skłonny doradzić recenzentom, aby dokonali większego refaktoryzacji. We wszystkich przypadkach nie oczekuj bezbłędnego, czystego fragmentu kodu z ładną architekturą i designem w kiepskiej bazie kodu. To, czego nie powinieneś zachęcać, to zachowanie, w którym dobrze poinformowany programista, który musi pracować nad kiepską bazą kodu, stara się dobrze wykonać swoją rolę . Zamiast upraszczać, tylko komplikuje je przedtem. Teraz, zamiast jednolitego, złego kodu, masz część ze wzorami projektowymi, inną część z czystym, przejrzystym kodem, inną część, która z biegiem czasu została gruntownie zreformowana, i nie ma żadnej jedności.

Wyobraź sobie na przykład, że odkrywasz starszą bazę kodów witryny średniej wielkości. Jesteś zaskoczony brakiem jakiejkolwiek zwykłej struktury i faktem, że rejestrowanie, gdy jest zrobione, polega na ręcznym dodawaniu elementów do pliku tekstowego, zamiast korzystania z frameworka rejestrowania. Ty decydujesz, aby nowa funkcja korzystała z MVC i środowiska rejestrowania.

Twój kolega wdraża kolejną funkcję i jest bardzo zaskoczony brakiem ORM, w którym można uzyskać idealny rozmiar. Więc zaczyna używać ORM.

Ani ty, ani twój kolega nie jesteście w stanie przejść przez setki tysięcy wierszy kodu, aby wszędzie korzystać z MVC, frameworka rejestrowania lub ORM. W rzeczywistości wymagałoby to miesięcy pracy: wyobraź sobie wprowadzenie MVC; ile to zajmie? A co z ORM w sytuacjach, w których zapytania SQL były chaotycznie generowane przez konkatenację (z okazjonalnymi miejscami dla SQL Injection) wewnątrz kodu, którego nikt nie rozumiał?

Myślisz, że wykonałeś świetną robotę, ale teraz nowy programista, który dołącza do projektu, musi stawić czoła znacznie większej złożoności niż wcześniej:

• Stary sposób traktowania próśb,

• Sposób MVC,

• Stary mechanizm rejestrowania,

• Ramy rejestrowania,

• Bezpośredni dostęp do bazy danych dzięki zapytaniom SQL budowanym w locie,

• ORM.

Przy jednym projekcie, nad którym pracowałem, były używane cztery (!) Struktury rejestrowania obok siebie (plus rejestrowanie ręczne). Powodem jest to, że za każdym razem, gdy ktoś chciał się zalogować, nie było wspólnego podejścia do tego, więc zamiast uczyć się nowego frameworka (który we wszystkich przypadkach był używany tylko w 5% bazy kodu), wystarczy dodać kolejny już wie. Wyobraź sobie bałagan.

Lepszym rozwiązaniem byłoby przefakturowanie bazy kodu krok po kroku. Biorąc jeszcze raz przykład rejestrowania, refaktoryzacja składałaby się z następujących małych kroków:

• Znajdź wszystkie miejsca, w których odbywa się rejestrowanie starszego typu (tj. Gdy plik dziennika jest uzyskiwany bezpośrednio) i upewnij się, że wszystkie wywołują te same metody.

• Przenieś ten kod do dedykowanej biblioteki, jeśli dotyczy. Nie chcę logować pamięci masowej w mojej klasie koszyka.

• W razie potrzeby zmień interfejs metod rejestrowania. Na przykład możemy dodać poziom wskazujący, czy wiadomość jest nieformalna, czy jest ostrzeżeniem lub błędem.

• Użyj nowo zreformowanych metod w nowej funkcji.

• Przeprowadź migrację do środowiska rejestrowania: jedynym kodem, którego dotyczy problem, jest kod z dedykowanej biblioteki.

Jeśli przeglądasz kod w ramach procesu programowania; następnie musisz ustalić zasady, według których „oceniasz” kod, który przeglądasz.

Powinno to być zgodne z „definicją ukończenia” i może być przewodnikiem po stylu, dokumentem architektury bazy danych lub analizą statyczną, sprawdzającym wymagania prawne, niezależnie od tego, co firma zdecyduje o wymaganiach dotyczących „jakości kodu”

Po wprowadzeniu tego przeglądu recenzje stają się oczywiste, czy przestrzegano przewodnika po stylach, czy mamy wymagany procent pokrycia testowego itp.

Jeśli nie masz tego na swoim miejscu, recenzje kodu mogą stać się po prostu bitwą o to, kto ma największe kodowanie lub, podobnie jak w twojej sytuacji, kwestionowaniem wezwań do oceny refaktoryzacji vs funkcji wykonanych przed upływem terminu. Co jest stratą czasu każdego.

Twoim głównym problemem jest to, że przegląd kodu znaczącej nowej funkcji jest nieodpowiednim czasem na rozmowę. W tym momencie jest już za późno, aby dokonać drobnych zmian. Właściwe miejsce znajduje się na etapie planowania lub najpóźniej we wstępnym przeglądzie projektu. Jeśli Twoja firma nie przeprowadza tych wczesnych recenzji, przynajmniej nieformalnie, powinieneś najpierw zmienić tę kulturę.

Następnym krokiem jest zaproszenie Cię na te spotkania i produktywne pomysły na tych spotkaniach. Przeważnie oznacza to, że nie próbujesz zmieniać wszystkiego z dnia na dzień, ale szukasz kawałków wielkości kęsa, które możesz izolować i rozwiązywać. Te fragmenty z czasem znacznie się zsumują.

Innymi słowy, kluczem do sukcesu jest regularne sugerowanie mniejszych zmian na początku projektów, a nie rezygnacja z sugerowania większych zmian pod koniec.

W miejscach, w których robiłem przegląd kodu, zaakceptowałbym i zatwierdziłbym przesłanie kodu, jeśli wiąże się to ze zobowiązaniem do (przynajmniej) częściowego refaktoryzacji. Czy to jako zgłoszony błąd, jako historia, czy też obietnica wysłania kolejnej recenzji z (niektórymi) dokonanymi refaktoryzacjami.

W takich przypadkach, jeśli jestem osobą, która pisze kod do recenzji, zwykle przygotowuję dwie zmiany, jedną z moimi nowymi funkcjami lub poprawkami błędów, a następnie drugą, która ma ORAZ pewne poprawki. W ten sposób zmiany czyszczenia nie zmniejszają nowych funkcji ani poprawek, ale można je łatwo wskazać jako token „tak, wiem, że to nie rozwiązuje tych problemów, ale tam jest” czyste „czyszczenie”.