Przygotowuję wytyczne dotyczące recenzji kodu. Nie mamy jeszcze jednego formalnego procesu i próbujemy go sformalizować. Nasz zespół jest rozproszony geograficznie.
Używamy TFS do kontroli źródła (używaliśmy go również do zadań / śledzenia błędów / zarządzania projektami, ale przeprowadziliśmy migrację do JIRA ) z Visual Studio 2008 do programowania.
Jakich rzeczy szukasz podczas przeglądania kodu?
- To są rzeczy, które wymyśliłem
- Egzekwuj reguły FxCop (jesteśmy sklepem Microsoft)
- Sprawdź wydajność (jakieś narzędzia?) I bezpieczeństwo (myśląc o użyciu OWASP - przeszukiwacza kodu ) oraz bezpieczeństwo wątków
- Przestrzegaj konwencji nazewnictwa
- Kod powinien obejmować przypadki brzegowe i warunki brzegowe
- Powinien poprawnie obsługiwać wyjątki (nie połykać wyjątków)
- Sprawdź, czy funkcjonalność jest powielona w innym miejscu
- Ciało metody powinno być małe (20-30 linii), a metody powinny robić jedną rzecz i tylko jedną rzecz (bez efektów ubocznych i unikać czasowego łączenia -)
- Nie przekazuj / nie zwracaj wartości null w metodach
- Unikaj martwego kodu
- Dokumentuj publiczne i chronione metody / właściwości / zmienne
Na jakie inne rzeczy powinniśmy na ogół uważać?
Próbuję sprawdzić, czy potrafimy skwantyfikować proces recenzji (dałby identyczne dane wyjściowe, gdy zostałby sprawdzony przez różne osoby) Przykład: Powiedzenie „treść metody nie powinna być dłuższa niż 20-30 linii kodu” w przeciwieństwie do powiedzenia „metoda ciało powinno być małe ".
Czy też weryfikacja kodu jest bardzo subiektywna (i różni się od jednego recenzenta do drugiego)?
Celem jest posiadanie systemu znakowania (powiedzmy -1 punkt za każde naruszenie reguły FxCop, -2 punkty za nieprzestrzeganie konwencji nazewnictwa, 2 punkty za refaktoryzację itp.), Aby programiści byli bardziej ostrożni, gdy sprawdzali swój kod. W ten sposób możemy zidentyfikować programistów, którzy konsekwentnie piszą dobry / zły kod. Celem jest, aby recenzent spędził maksymalnie 30 minut, aby dokonać przeglądu (wiem, że jest to subiektywne, biorąc pod uwagę fakt, że zestaw zmian / wersja może zawierać wiele plików / ogromne zmiany w istniejącej architekturze itp., Ale otrzymujesz ogólnie rzecz biorąc, recenzent nie powinien spędzać dni na sprawdzaniu czyjegoś kodu).
Jaki inny obiektywny / wymierny system stosuje się do identyfikowania dobrego / złego kodu napisanego przez programistów?
Odnośnik do książki: Clean Code: Podręcznik zwinnego kunsztu programistycznego autorstwa Roberta Martina .
źródło
this.Foo().FooBar().FooBarBar();
Jeśli obiekt zwrócony tutaj przez Foo ma wartość NULL, zdecydowanie możesz uniknąć „Odwołanie do obiektu nie ustawione na instancję obiektu”, wywołując FooBar ()Odpowiedzi:
Ocenianie osób w przeglądzie jest sprzeczne z najbardziej udanymi systemami, z którymi pracowałem, a może wszystkim. Ale celem, który staram się osiągnąć od ponad 20 lat, jest zmniejszenie liczby błędów i zwiększenie wydajności na godzinę pracy inżyniera. Jeśli ocenianie osób jest celem, przypuszczam, że można by użyć recenzji. Nigdy nie widziałem sytuacji, w której byłby potrzebny, jako pracownik lub lider.
Niektóre obiektywne badania (Fagan itp.) I wiele popularnych mądrości sugeruje, że relacje rówieśnicze ułatwiają przeglądanie kodu w celu ograniczenia błędów i zwiększenia wydajności. Pracujący kierownicy mogą brać udział jako pracownicy, ale nie jako kierownicy. Podkreślono punkty dyskusji, zmiany w celu zadowolenia recenzentów są na ogół dobrą rzeczą, ale nie są wymagane. Stąd relacja rówieśnicza.
Wszelkie zautomatyzowane narzędzia, które można zaakceptować bez dalszej analizy lub osądu, są dobre - Cint , C ++, Java. Regularna kompilacja. Kompilatory są NAPRAWDĘ dobre w znajdowaniu błędów kompilatora. Dokumentowanie odchyleń w automatycznych czekach brzmi jak subtelne oskarżenie o automatyczne kontrole. Dyrektywy kodowe (podobnie jak Java), które dopuszczają odchylenia, są dość niebezpieczne, IMHO. Idealne do debugowania, aby szybko uzyskać sedno sprawy. Nie tak dobrze jest znaleźć w źle udokumentowanym, 50 000 wierszu kodu bez wiersza komentarza, za który stałeś się odpowiedzialny.
Niektóre zasady są głupie, ale łatwe do egzekwowania; domyślne dla każdej instrukcji switch, nawet jeśli są nieosiągalne, na przykład. To tylko pole wyboru i nie musisz tracić czasu i pieniędzy na testowanie z wartościami, które niczego nie pasują. Jeśli masz zasady , będziesz głupotą , są nierozerwalnie powiązane . Każda korzyść z reguły powinna być warta głupoty, którą to kosztuje, a związek ten powinien być sprawdzany w regularnych odstępach czasu.
Z drugiej strony „biegnie” nie ma cnoty przed przeglądem ani obrony w przeglądzie. Jeśli rozwój postępował zgodnie z modelem wodospadu , chciałbyś dokonać przeglądu, gdy kodowanie zakończy się w 85%, zanim zostaną znalezione i opracowane skomplikowane błędy, ponieważ przegląd jest tańszym sposobem na ich znalezienie. Ponieważ prawdziwe życie nie jest modelem wodospadu, kiedy przeglądanie jest czymś w rodzaju sztuki i jest normą społeczną. Ludzie, którzy faktycznie czytają Twój kod i szukają w nim problemów, są z litego złota. Zarządzanie, które wspiera to na bieżąco, to perła ponad cenę. Recenzje powinny przypominać meldowanie się - wcześnie i często .
Uważam, że te rzeczy są korzystne:
1) Brak wojen stylowych . Tam, gdzie idą otwarte nawiasy klamrowe, należy sprawdzać spójność tylko w danym pliku. Wszystkie takie same. W porządku. Podobnie głębokość wcięcia ** si ** szerokości zakładek . Większość organizacji odkrywa, że potrzebuje wspólnego standardu tab, który jest używany jako duża przestrzeń.
2) `Obdarty
tekst, który nie ma
dla treści.`
BTW, K&R wcięły pięć (PIĘĆ) spacji, więc apele do autorytetu są bezwartościowe. Po prostu bądź konsekwentny.
3) Przed przeglądem należy podać numer, niezmienną, publicznie dostępną kopię pliku do przejrzenia na co najmniej 72 godziny.
4) Brak projektowania w locie. Jeśli występuje problem lub problem, zanotuj jego lokalizację i ruszaj się.
5) Testowanie wszystkich ścieżek w środowisku programistycznym jest bardzo, bardzo, bardzo dobrym pomysłem. Testy, które wymagają ogromnych danych zewnętrznych, zasobów sprzętowych, korzystania z witryny klienta itp., To testy, które kosztują fortunę i nie będą dokładne.
6) Format pliku inny niż ASCII jest dopuszczalny, jeśli narzędzia do tworzenia, wyświetlania, edycji itp. Istnieją lub są tworzone na wczesnym etapie rozwoju. To moja osobista stronniczość, ale w świecie, w którym dominujący system operacyjny nie jest w stanie wyjść z siebie z mniej niż 1 gigabajtem pamięci RAM, nie rozumiem, dlaczego pliki mniejsze niż, powiedzmy, 10 megabajtów powinny być czymkolwiek inny niż ASCII lub inny format obsługiwany komercyjnie. Istnieją standardy grafiki, dźwięku, filmów, plików wykonywalnych i towarzyszących im narzędzi. Nie ma usprawiedliwienia dla pliku zawierającego binarną reprezentację pewnej liczby obiektów.
W celu konserwacji, refaktoryzacji lub opracowania zwolnionego kodu jedna grupa współpracowników wykorzystałem recenzję przez jedną inną osobę, siedzącą przy wystawie i patrzącą na różnicę między starym a nowym , jako bramą do odprawy oddziału. Podobało mi się, było tanie, szybkie, stosunkowo łatwe do zrobienia. Wskazówki dla osób, które nie przeczytały wcześniej kodu, mogą być pouczające dla wszystkich, ale rzadko poprawiają kod programisty.
Jeśli jesteś rozproszony geograficznie, patrzenie na różnice na ekranie podczas rozmowy z kimś innym patrząc na to samo byłoby stosunkowo łatwe. Obejmuje to dwie osoby patrzące na zmiany. W przypadku większej grupy, która przeczytała dany kod, wiele witryn nie jest trudniejsze niż wszystkie w jednym pokoju. Wiele pokoi połączonych wspólnymi ekranami komputerowymi i squak-boxami działa bardzo dobrze, IMHO. Im więcej witryn, tym bardziej potrzebne jest zarządzanie spotkaniami. Menedżer jako facylitator może tutaj zarobić na utrzymanie. Pamiętaj, aby nadal odpytywać witryny, w których Cię nie ma.
W pewnym momencie ta sama organizacja przeprowadziła automatyczne testy jednostkowe, które wykorzystano jako testy regresyjne. To było naprawdę miłe. Oczywiście zmieniliśmy wtedy platformy i automatyczny test został pominięty. Recenzja jest lepsza, jak zauważa Manifest Agile , relacje są ważniejsze niż proces lub narzędzia . Ale po przejrzeniu opinii automatyczne testy jednostkowe / testy regresji są kolejną najważniejszą pomocą w tworzeniu dobrego oprogramowania.
Jeśli możesz oprzeć testy na wymaganiach , cóż, jak mówi dama w „When Harry Met Sally” , będę miał to, co ona ma!
Wszystkie recenzje muszą mieć parking, aby uchwycić wymagania i problemy projektowe na poziomie powyżej kodowania. Gdy coś zostanie uznane na parkingu, dyskusja powinna się zakończyć.
Czasami myślę, że przegląd kodu powinien być jak przegląd schematów w projektowaniu sprzętu - całkowicie publiczny, dokładny, samouczek, koniec procesu, brama, po której jest budowany i testowany. Ale przeglądy schematyczne są bardzo ciężkie, ponieważ zmiana fizycznych obiektów jest droga. Przegląd architektury, interfejsu i dokumentacji oprogramowania powinien być prawdopodobnie ciężki. Kod jest bardziej płynny. Przegląd kodu powinien być lżejszy.
Myślę, że pod wieloma względami technologia dotyczy zarówno kultury i oczekiwań, jak i konkretnego narzędzia. Pomyśl o wszystkich improwizacjach „ Swiss Family Robinson ” / Flintstones / McGyver, które zachwycają serce i rzucają wyzwanie umysłowi. Chcemy, żeby nasze rzeczy działały . Nie ma do tego ani jednej ścieżki, podobnie jak „inteligencja”, którą można by jakoś wyabstrahować i zautomatyzować za pomocą programów AI z lat 60 .
źródło
Większość opisanych przez ciebie punktów to tylko kwestia formatowania lub „powierzchownych” rzeczy:
Wszystko to można sprawdzić za pomocą zautomatyzowanego narzędzia : nie ma potrzeby, aby doświadczony programista spędzał czas na przeglądaniu kodu, aby na to uważać.
W ogóle nie wiem o .NET , ale dla PHP mamy narzędzia do sprawdzania tego rodzaju rzeczy; biorąc pod uwagę, że .NET często mówi się, że jest „bardziej przemysłowy” niż PHP, zdziwiłbym się, słysząc, że nie ma żadnego narzędzia do sprawdzania tego rodzaju rzeczy.
Zautomatyzowane narzędzie może zarówno:
Poczta może zostać wysłana do całego zespołu lub do faceta, który popełnił kod, który nie przejdzie testu - lub możesz użyć interfejsu internetowego do raportowania (ta sama uwaga na temat .NET i PHP)
Dodałbym również, że automatyczne testowanie może bardzo pomóc w wykryciu pewnej liczby błędów, zanim kod zostanie użyty w produkcji. Zautomatyzowane testy mogą również pomóc w niektórych pomiarach.
Recenzje kodu wykonane przez doświadczonych programistów mają także kolejną wielką zaletę, o której nie mówiłeś:
Ale do przeglądu kodu, który idzie głębiej niż tylko formatowanie kodu, potrzebujesz więcej niż pół godziny ...
źródło
Z moich doświadczeń związanych z przeglądaniem kodu wynika, że powinien to być wspólny wysiłek na rzecz ulepszenia kodu, a nie „środek” decydujący o tym, kto jest dobry czy zły w swojej pracy. Jeśli nie ma znaczenia, czy podczas sprawdzania kodu pojawi się wiele uwag, recenzenci będą bardziej rygorystyczni, dlatego będą sugerować ulepszenia kodu.
Aby poprawić jakość sprawdzanego kodu, wymuszaj przetwarzanie komentarzy recenzji (pozwól recenzentowi zatwierdzić przetworzone komentarze), a także użyj narzędzi do sprawdzania kodu statycznego, aby wymusić poziom jakości dla początkowego zatwierdzenia.
źródło
Myślę, że twój system oceniania to zły pomysł. O co chodzi? Aby zidentyfikować dobrych i złych programistów? Każdy w tym przeglądzie kodu może lepiej ocenić niektóre programiści na podstawie kodu przedstawionego w przeglądzie kodu niż dowolne przypisanie wartości do nieco arbitralnego zestawu cech. Jeśli chcesz zidentyfikować dobrych i złych programistów ... zapytaj programistów. Gwarantuję, że ludzie mogą dokonać tej oceny lepiej niż twoja głupia heurystyka.
Moją propozycją byłoby ulepszenie recenzji kodu, aby ludzie otwarcie dzielili się pomysłami i opiniami w środowisku pozbawionym osądów i wrogim. Jeśli możesz to zrobić, będziesz 100 razy lepszy niż ocenianie programistów na podstawie twoich głupich list kontrolnych, które mają na celu dobrą ocenę programistów. Myślę, że wielu programistów jest już z siebie dumnych i trudnych, jeśli źle radzą sobie z recenzjami kodu; Zastanawiam się, czy dalsza „kara” za słabe wyniki jest ogólnie pomocna.
źródło
Moją jedyną radą byłoby uniknięcie zbyt rygorystycznego procesu sprawdzania kodu - najważniejsze jest to, że przegląd kodu rzeczywiście się dzieje i jest traktowany poważnie .
Im bardziej wyczerpujący jest proces dla recenzenta, tym mniej prawdopodobne jest, że recenzje kodu będą miały miejsce i że będą one traktowane poważnie, a nie tylko jako uciążliwość. Poza tym prawdziwa wartość recenzji kodu polega na tym, że recenzent może użyć własnego osądu, za pomocą zautomatyzowanych narzędzi można sprawdzić takie rzeczy, jak to, czy reguły FXCop miną.
źródło
Zasadniczo unikaj spędzania czasu na przeglądaniu kodu, robiąc coś, co można zrobić za pomocą komputera. Na przykład, Twoim pierwszym celem jest „egzekwowanie zasad FxCop”, ale przypuszczalnie może to zrobić FxCop bez konieczności, aby ludzie to robili.
źródło
Jeśli możesz to zmierzyć, jeśli jest to obiektywne, kwantyfikowalne, spróbuj użyć narzędzia. Tam, gdzie potrzebujesz doświadczonego recenzenta, to rozmyte subiektywne rzeczy.
źródło
Wiele dobrych komentarzy zostało już napisanych na temat stylu, co jest ważne. W projekcie zespołowym warto, aby cały kod wyglądał tak, jakby został napisany przez jednego autora. Ułatwia to innym członkom zespołu wpadanie i naprawianie problemów, gdy się pojawią. Jakie środki ilościowe wybierzesz, aby zapewnić ten szerszy cel, są mniej ważne.
Dodatkowym elementem jest zapewnienie zgodności kodu z ogólną architekturą uzgodnioną dla reszty systemu. Podobne problemy należy rozwiązać w ten sam sposób. Jeśli logika aplikacji została podzielona na warstwy, czy sprawdzany kod rozbija jego funkcjonalność tak, jak robi to reszta systemu? Alternatywnie, czy testowany kod uczy czegoś nowego, co powinno zostać wycofane w pozostałej części systemu? Podobnie jak kontrole stylu zapewniają, że kod wygląda tak samo, przegląd architektury powinien zapewnić, że kod działa w ten sam sposób. Ponownie nacisk kładziony jest na łatwość konserwacji. Każdy członek zespołu powinien mieć możliwość zapoznania się z tym kodem i natychmiastowego zapoznania się z tym, co się dzieje.
Pomysł oceniania wydaje się katastrofą, ale znasz swój zespół najlepiej. Możliwe, że będą motywowani przez taki system, ale myślę, że bardziej prawdopodobne jest, że ludzie zaczną się bardziej martwić o swoją ocenę niż rozwiązywanie problemów. Jednym z naprawdę cennych efektów ubocznych przeglądów kodu są oferowane przez nich możliwości mentoringu. Recenzent powinien traktować osobę, która napisała kod, jako osobę, której mentoruje. Każdy znaleziony problem nie stanowi problemu, ale jest szansą na stworzenie bardziej kompetentnego i wyrafinowanego członka zespołu oraz ściślejszego zespołu.
źródło
Szczerze mówiąc, bardziej obchodzą mnie rzeczy „subiektywne” niż cokolwiek innego. Po dobrej recenzji kodu chcę, aby ktoś sprawdził moją logikę, a nie pisanie. I na tym się skupiam, kiedy recenzuję kod.
Ogólny format, który lubię, to:
Bez tego samo patrzenie na różnice zwykle daje wkład w drobne problemy lub punkty stylistyczne. Bardziej martwi mnie to, czy logika jest poprawna, ogólnie przyjęte podejście jest dobre i czy rozwiązanie będzie możliwe do utrzymania.
Jako przykład ostatnio spojrzałem na jakiś kod współpracownika. Pierwotny problem dotyczył naruszenia FxCop. Ale próbowano ustalić obecność lub brak funkcji systemu Windows, sprawdzając numer wersji. Mój główny wkład polegał na tym, że był to kruchy sposób, aby to zrobić, i lepiej byłoby odpytywać usługę bezpośrednio, ponieważ mapowanie między istnieniem funkcji a SKU systemu Windows może się zmienić w przyszłości i wcale nie jest przyszłościowe.
źródło
Cyklomatyczna złożoność (CC) jest jednym ze sposobów oceny kodu, który „nie jest zły”.
W rzeczywistym kodzie, który ma wysokie CC, mam wysoki współczynnik „co tu się dzieje, nie pamiętam”. Niższy kod CC jest łatwiejszy do zrozumienia.
Oczywiście, zwykłe zastrzeżenia dotyczą metryk.
źródło
Przeglądy kodu są zarówno subiektywne, jak i obiektywne. Reguły takie jak „treść metody musi wynosić 20-30 linii” są subiektywne (niektóre osoby mogą myśleć, że 100 linii jest w porządku), ale jeśli Twoja firma zdecydowała, że limit 20-30 linii jest w porządku, możesz to zmierzyć. Myślę, że wymyślony przez ciebie system punktowy to świetny pomysł. Będziesz musiał okresowo dokonywać ponownej oceny, ponieważ okaże się, że niektóre reguły muszą mieć mniejszą lub większą wagę w punktacji, ale dopóki wszyscy znają reguły, wygląda to na dobry system.
Inne rzeczy, których bym szukał:
źródło
Wygląda na to, że zbyt szybko stajesz się zbyt szczegółowy. Powinieneś to bardziej rozbić. Należy przestrzegać kodu pod kątem jego jakości i zgodności funkcji. Powinieneś się rozdzielić i to nawet nie koniec historii ... oto moja propozycja:
Jakość kodu:
Zgodność funkcji:
Jeśli potrafisz opisać dwa aspekty przeglądu kodu, jesteś złoty.
źródło
Mógłbym napisać kilka akapitów, ale chciałbym tylko przelotnie omówić to, co Karl Wiegers wyjaśnia w „Recenzjach w oprogramowaniu: praktyczny przewodnik” . Myślę, że jego książka zawiera wyraźne i zwięzłe odpowiedzi na twoje pytanie (i wiele więcej).
źródło
To zależy.
Niektóre części recenzji można łatwo obliczyć (bez problemów z FxCop, bez błędów StyleCop , bez błędów CAT.NET itp.)
Styl może być jednak subiektywny - ale jak mówisz, kiedy zaczniesz być bardziej szczegółowy (bez metody> 20 linii), możesz go zmierzyć, a narzędzia takie jak NDepend mogą to zrobić automatycznie. Niektóre rzeczy nigdy nie będą automatyczne - sprawdzanie obsługi przypadków na krawędzi wymagałoby testów, co powoduje zwiększenie zasięgu kodu, a 100% jest nieosiągalnym ideałem w wielu przypadkach. Sprawdzanie duplikacji jest trudne do wykonania automatycznie. Czeki zerowe, cóż, nie jestem pewien, czy się z tobą zgadzam, ale możesz być w stanie napisać reguły NDepend lub reguły FxCop dla tego.
Im więcej narzędzi, tym lepiej, a jeśli narzędzia pozwalają programistom sprawdzić swoją pracę przed zatwierdzeniem zmian i wykonać kontrole w ramach procesu CI , to zminimalizujesz potrzebę recenzji.
źródło
System znakowania wydaje się trudny do poprawnego wykonania, ale warto go mieć jako narzędzie pomiarowe: nie można poprawić tego, czego nie można zmierzyć. Ale prawdopodobnie powinieneś zaakceptować fakt, że pewne rzeczy będą trudne / niemożliwe do dokładnego oszacowania. Trudne będzie ustalenie, ile punktów powinna uzyskać każda jakość: na przykład, jeśli przestrzeganie konwencji nazewnictwa daje 2 punkty, to ile punktów za utrzymanie metod na niskim poziomie?
Być może coś w rodzaju prostej listy kontrolnej byłoby lepsze, aby kod mógł zostać oznaczony jako zgodny, częściowo zgodny lub niezgodny z określoną jakością. Później możesz dodać ocenę do listy kontrolnej, gdy zobaczysz, jakie problemy z jakością pojawiają się najczęściej lub powodują najwięcej problemów.
Proces przeglądu powinien być również na tyle elastyczny, aby kod mógł zawieść w części przeglądu, pod warunkiem że można to uzasadnić i udokumentować. Ślepe trzymanie się jakiegoś standardu jakości kodu, który powoduje, że komponent staje się niepotrzebnie skomplikowany / niemożliwy do zarządzania, jest złym pomysłem!
źródło
Jeśli chcesz, aby kod ludzi był bardziej znormalizowany, nie zmuszając go do „marnowania czasu na formatowanie”, na co narzekają niektórzy programiści. Zainwestuj w narzędzie takie jak ReSharper, ponieważ sprawia, że naprawianie formatowania i innych zadań związanych z faktoringiem jest prawie zautomatyzowanym procesem.
źródło
źródło