Czy wbudowane komentarze „edytowane przez” są normą w sklepach korzystających z kontroli wersji?

17

Starszy programista w naszym sklepie nalega, aby za każdym razem, gdy kod był modyfikowany, odpowiedzialny programista powinien dodawać wbudowany komentarz stwierdzający, co zrobił. Te komentarze zwykle wyglądają// YYYY-MM-DD <User ID> Added this IF block per bug 1234.

Używamy TFS do kontroli wersji i wydaje mi się, że tego rodzaju komentarze są o wiele bardziej odpowiednie jako notatki przy odprawie niż wewnętrzny szum. TFS pozwala nawet powiązać odprawę z co najmniej jednym błędem. Niektóre z naszych starszych, często modyfikowanych plików klas wyglądają tak, jakby miały stosunek komentarza do LOC zbliżony do 1: 1. Moim zdaniem komentarze te utrudniają odczytanie kodu i dodają wartość zerową.

Czy jest to standardowa (lub przynajmniej powszechna) praktyka w innych sklepach?

Joshua Smith
źródło
12
Zgadzam się z tobą, po to są dzienniki wersji w kontroli wersji.
TZHX
1
Starszy programista w twoim zespole zrobił to, zanim kontrola wersji była szeroko rozpowszechniona, i nie doszedł do wniosku, że należy usunąć go z innych źródeł. Pokaż mu sześć projektów open source wykorzystujących system Bugzilli i VC, zapytaj go, czy powinien wiedzieć w komentarzach biblioteki jQuery, że $ .ajax () został ostatnio zmieniony przez jaubourg 4 miesiące temu i wszystkie drobne zmiany stworzone przez setki ludzi, również tam są. Cała biblioteka jQuery byłaby bałaganem komentarzy i nic nie zostało zdobyte!
Incognito
1
W rzeczywistości mamy niepisaną zasadę braku nazw w kodzie źródłowym, ponieważ może to stworzyć poczucie własności kodu, które jest uważane za BadThing TM.
Stephen Paulger,

Odpowiedzi:

23

Zazwyczaj uważam takie komentarze za złą praktykę i myślę, że tego rodzaju informacje należą do dzienników zatwierdzania SCM. W większości przypadków sprawia to, że kod jest trudniejszy do odczytania.

Jednakże , nadal często zrobić coś takiego dla określonych rodzajów zmian.

Przypadek 1 - zadania

Jeśli używasz środowiska IDE, takiego jak Eclipse, Netbeans, Visual Studio (lub masz jakiś sposób na wyszukiwanie tekstu w bazie kodu za pomocą czegokolwiek innego), być może twój zespół używa określonych „tagów komentarzy” lub „tagów zadań”. W takim przypadku może to być przydatne.

Od czasu do czasu podczas przeglądania kodu dodaję coś takiego:

// TOREVIEW: [2010-12-09 haylem] marking this for review because blablabla

lub:

// FIXME: [2010-12-09 haylem] marking this for review because blablabla

Używam do tego różnych niestandardowych tagów zadań, które widzę w Eclipse w widoku zadań, ponieważ posiadanie czegoś w dziennikach zatwierdzeń jest dobrą rzeczą, ale niewystarczającą, gdy na spotkaniu przeglądowym ktoś pyta cię, dlaczego poprawka XY została całkowicie zapomniana i prześlizgnął się. Tak więc w pilnych sprawach lub naprawdę wątpliwych fragmentach kodu służy to jako dodatkowe przypomnienie (ale zwykle skracam komentarz i sprawdzam dzienniki zatwierdzeń, ponieważ właśnie po to jest przypomnienie, więc nie zaśmiecam też kodu wiele).

Przypadek 2 - łaty Libs stron trzecich

Jeśli mój produkt musi spakować fragment kodu innej firmy jako źródło (lub bibliotekę, ale przebudowany ze źródła), ponieważ z jakiegoś powodu musiał zostać załatany, dokumentujemy łatkę w osobnym dokumencie, w którym wymieniliśmy te „zastrzeżenia” do wykorzystania w przyszłości, a kod źródłowy zwykle zawiera komentarz podobny do:

// [PATCH_START:product_name]
//  ... real code here ...
// [PATCH_END:product_name]

Przypadek 3 - nieoczywiste poprawki

Ten jest nieco bardziej kontrowersyjny i bliższy temu, o co prosi twój starszy programista.

W produkcie, nad którym obecnie pracuję, czasami (zdecydowanie nie jest to coś powszechnego) mamy komentarz:

// BUGFIX: [2010-12-09 haylem] fix for BUG_ID-XYZ

Robimy to tylko wtedy, gdy poprawka nie jest oczywista, a kod jest odczytywany nienormalnie. Może tak być na przykład w przypadku dziwactw przeglądarki lub niejasnych poprawek CSS, które należy wdrożyć tylko dlatego, że w produkcie występuje błąd dokumentu. Ogólnie rzecz biorąc, chcielibyśmy połączyć go z naszym wewnętrznym repozytorium problemów, które następnie będzie zawierało szczegółowe uzasadnienie poprawki i wskazówki do dokumentacji błędu zewnętrznego produktu (powiedzmy, poradę bezpieczeństwa dla znanej wady Internet Explorera 6 lub coś w tym stylu).

Ale jak wspomniano, jest to dość rzadkie. A dzięki tagom zadań możemy regularnie je przeglądać i sprawdzać, czy te dziwne poprawki nadal mają sens lub czy można je wycofać (na przykład, jeśli porzuciliśmy obsługę wadliwego produktu powodującego błąd).


To właśnie w: przykład z prawdziwego życia

W niektórych przypadkach jest to lepsze niż nic :)

Właśnie natrafiłem na ogromną klasę obliczeń statystycznych w mojej bazie kodu, w której komentarz nagłówka miał postać dziennika zmian ze zwykłym yadda yadda: recenzent, data, identyfikator błędu.

Na początku myślałem o złomowaniu, ale zauważyłem, że identyfikatory błędów nie tylko nie zgadzają się z konwencją naszego obecnego narzędzia do śledzenia problemów, ale także nie pasują do tego, z którego korzystałem, zanim dołączyłem do firmy. Próbowałem więc przeczytać kod i zrozumieć, co robi klasa (nie będąc statystyką), a także próbowałem wykopać te raporty błędów. Zdarzyło się, że były one dość ważne i miałyby kłopot z życiem następnego faceta, aby edytować plik, nie wiedząc o nich dość okropnie, ponieważ zajmował się drobnymi problemami z precyzją i specjalnymi przypadkami opartymi na bardzo specyficznych wymaganiach wydawanych przez pierwotnego klienta . Ostatecznie, gdyby ich tam nie było, nie wiedziałbym. Gdyby ich tam nie było, a ja lepiej zrozumiałbym klasę,

Czasami trudno jest śledzić bardzo stare wymagania, takie jak te. Ostatecznie to, co zrobiłem, to nadal usuwanie nagłówka, ale po skradnięciu się w bloku komentarza przed każdą obciążającą funkcją opisującą, dlaczego te „dziwne” obliczenia są konkretnymi żądaniami.

W takim razie nadal uważałem to za złą praktykę, ale chłopcze, byłem szczęśliwy, że oryginalny twórca przynajmniej je zastosował! Lepiej byłoby zamiast tego wyraźnie skomentować kod, ale wydaje mi się, że było to lepsze niż nic.

Haylem
źródło
Te sytuacje mają sens. Dzięki za wkład.
Joshua Smith
drugi przypadek to zwykły komentarz do kodu, dlaczego w kodzie jest łatka. pierwszy przypadek jest poprawny: tylko uwaga, że ​​kod nie jest skończony lub jest jeszcze trochę do zrobienia w tej części.
Salandur
Cóż, starszy programista w moim miejscu pracy (ja) mówi, że zmiany wprowadzają komentarze w systemie zarządzania konfiguracją oprogramowania. Masz połączony SCM i narzędzia do śledzenia defektów, prawda? (Google SCMBUG) Nie rób ręcznie tego, co można zautomatyzować.
Tim Williscroft
@Tim: Cóż, zgadzam się z tobą, jak mówi pierwsza linijka odpowiedzi. Ale w nieoczywistych przypadkach programiści (przypuszczam, że z lenistwa, ponieważ nie chcą tracić na to czasu) nie sprawdzą dzienników zatwierdzeń i przegapią niektóre kluczowe informacje, podczas gdy komentarz 10char może wskazać im właściwy identyfikator błędu, który sam będzie zawierał dzienniki wersji dotyczących tego błędu, jeśli skonfigurowałeś SCM i moduł śledzenia do współpracy. Naprawdę najlepszy ze wszystkich światów.
haylem
Z mojego doświadczenia wynika, że ​​komunikaty zatwierdzania są często pomijane (pomimo zasad, które je zawierają, jeśli w ogóle istnieją) lub są bezużyteczne (tylko numer problemu, a często zły). Ci sami ludzie zostawiają jednak komentarze w kodzie ... Wolę mieć te i brak komunikatu zatwierdzenia niż nic w ogóle (aw wielu środowiskach pracowałem nad odczytaniem komunikatów zatwierdzeń / historii źródła było tak trudne, że prawie niemożliwe , a nawet konieczność żądania źródeł od innych osób, ponieważ dostęp do kontroli wersji był ograniczony „ze względów bezpieczeństwa”).
jwenting
3

Używamy tej praktyki w przypadku plików nieobjętych kontrolą wersji. Mamy programy RPG działające na komputerach mainframe i okazało się, że trudniej jest zrobić znacznie więcej niż wykonać ich kopię zapasową.

W przypadku wersjonowanego kodu źródłowego korzystamy z uwag dotyczących odprawy. Myślę, że tam właśnie należą, a nie zaśmiecają kod. W końcu to metadane.

Michael K.
źródło
Zgadzam się, pliki niepodlegające kontroli wersji to inna historia.
Joshua Smith
1
„okazało się, że trudno zrobić o wiele więcej niż tworzenie kopii zapasowych”. Tak naprawdę to nie jest wymówka, że ​​mój CTO mógłby to znieść.
Tim Williscroft
@Tim: Zawsze otwarty na sugestie - mogę je podnieść w górę łańcucha dowodzenia. :) Ciężko jest pracować na komputerze mainframe, aby włączać i wyłączać pliki.
Michael K
Moja sugestia - prawdopodobnie możesz je zdjąć (kopia zapasowa); dlaczego nie po prostu to gdzieś rzucić i co noc wprowadzać zmiany w skryptach do mercurial?
Wyatt Barnett
2

Dawno temu mieliśmy tę praktykę w sklepie SW, gdzie nasz menedżer nalegał, aby chciał czytać pliki kodu na papierze i śledzić ich historię zmian. Nie trzeba dodawać, że nie pamiętam żadnej konkretnej okazji, kiedy rzeczywiście spojrzał na wydruk pliku źródłowego: - /

Na szczęście moi menedżerowie od tego czasu mieli większą wiedzę na temat tego, co mogą zrobić systemy kontroli wersji (muszę dodać, że korzystaliśmy z SourceSafe w tym pierwszym sklepie). Więc nie dodam metadanych wersji do kodu.

Péter Török
źródło
2

Zasadniczo nie jest wymagane, jeśli SCM i IDE pozwalają na użycie funkcji „pokaż adnotację” (zwaną tak w Eclipse), można łatwo zobaczyć, jakie zmiany zmieniły fragment kodu, a komentarze zmian powinny powiedzieć, kto i dlaczego.

Jedyny raz, kiedy umieszczam taki komentarz w kodzie, to jeśli jest to szczególnie dziwny fragment kodu, który może powodować zamieszanie w przyszłości, krótko skomentuję numer błędu, aby mogli przejść do raportu o błędzie i przeczytać szczegółowo o tym.

Alba
źródło
„nie musisz tego rozumieć” to prawdopodobnie zły komentarz do kodu. Znowu powiem, że połącz swój SCM i narzędzia do śledzenia błędów.
Tim Williscroft
2

Oczywiście wszelkie takie komentarze są niemal na pewno nieprawidłowe w czasie; jeśli edytujesz wiersz dwukrotnie, czy dodajesz dwa komentarze? Dokąd oni poszli? Dlatego lepszym procesem jest poleganie na swoich narzędziach.

Jest to znak, że z jakiegokolwiek powodu starszy programista nie może polegać na nowym procesie śledzenia zmian i łączenia zmian z systemem śledzenia problemów. Musisz rozwiązać ten problem, aby rozwiązać konflikt.

Musisz zrozumieć, dlaczego on nie może na tym polegać. Czy to stare nawyki? Złe doświadczenie z systemem SCM? Format prezentacji, który nie działa dla niego? Może nawet nie wie o narzędziach takich jak „git blame” i widoku osi czasu Perforce (który w zasadzie to pokazuje, choć może nie pokazywać, który problem spowodował zmianę).

Bez zrozumienia jego uzasadnienia nie będziesz w stanie go przekonać.

Alex Feinman
źródło
2

Pracuję na ponad 20-letnim produkcie Windows. Mamy podobną praktykę, która obowiązuje od dłuższego czasu. Nie mogę policzyć, ile razy ta praktyka uratowała mi bekon.

Zgadzam się, że niektóre rzeczy są zbędne. Kiedyś programiści używali tej praktyki do komentowania kodu. Kiedy to sprawdzam, mówię im, aby kontynuowali i usunęli kod, a komentarz nie jest konieczny.

Ale często zdarza się, że patrzysz na kod, który istniał około dekady i został zmodyfikowany przez 20 programistów, ale nigdy nie został zrefaktoryzowany. Wszyscy już dawno zapomnieli o wszystkich szczegółach wymagań zawartych w oryginalnym kodzie, nie wspominając o wszystkich zmianach i nie ma wiarygodnej dokumentacji.

Komentarz z numerem wady daje mi miejsce, w którym mogę sprawdzić pochodzenie kodu.

Tak, tak, system SCM robi dużo, ale nie wszystko. Traktuj je jak każdy inny komentarz i dodawaj je wszędzie tam, gdzie wprowadzona zostanie znacząca zmiana. Deweloper patrzy na Twój kod przez ponad 5 lat od tego, jak Ci podziękuje.


źródło
1

Wzmianka o każdej zmianie w komentarzach nie ma sensu, jeśli masz VCS. Przydatne jest wspomnienie o konkretnym błędzie lub innej ważnej uwadze związanej z określoną linią . Zmiana może obejmować wiele zmienionych linii, wszystkie w ramach tego samego komunikatu zatwierdzenia.

9000
źródło
1

Nie, że mogę powiedzieć.

W miarę upływu czasu rozpryskuję TODO w moim kodzie, a celem jest usunięcie ich do czasu przeglądu.

Paul Nathan
źródło