Czy złą praktyką jest nie usuwać zbędnych plików od razu z VCS, ale zamiast tego oznaczać je jako „Do usunięcia” z komentarzami?

53

Chciałem wiedzieć, czy sposób postępowania z plikami źródłowymi, które należy usunąć z kontroli wersji, można uznać za złą praktykę.

Chcę ci to wyjaśnić na podstawie tego przykładu:

Niedawno bardzo się zdenerwowałem, ponieważ musiałem żmudnie sortować klasy Java w programie, który był w zasadzie martwym kodem, jednak nigdzie go nie udokumentowano, a także nie skomentowano w tych klasach Java. Oczywiście trzeba je było usunąć, ale zanim usunę takie zbędne rzeczy, mam - niektórzy mogą powiedzieć dziwny - nawyk:

Nie usuwam takich zbędnych plików natychmiast za pomocą SVN-> Usuń (zamień na polecenie usuwania wybranego systemu kontroli wersji), ale umieszczam komentarze w tych plikach (odsyłam zarówno do nagłówka, jak i stopki), które zamierzają zostać usunięte + moje imię + data, a także - co ważniejsze - DLACZEGO SĄ USUNIĘTE (w moim przypadku, ponieważ były martwe, mylący kod). Następnie zapisuję i przekazuję je do kontroli wersji. Następnym razem, gdy muszę zatwierdzić / sprawdzić coś w projekcie do kontroli wersji, wciskam SVN-> Usuń, a następnie są one ostatecznie usuwane w Kontroli wersji - wciąż oczywiście można je przywrócić poprzez poprawki i dlatego przyjąłem ten nawyk.

Dlaczego warto to robić zamiast usuwać je od razu?

Moim powodem jest to, że chcę mieć wyraźne znaczniki przynajmniej w ostatniej wersji, w której istniały te zbędne pliki, dlaczego zasługiwały na usunięcie. Jeśli je od razu usunę, zostaną usunięte, ale nigdzie nie udokumentowano, dlaczego zostały usunięte. Chcę uniknąć typowego scenariusza takiego jak ten:

„Hmm… dlaczego te pliki zostały usunięte? Wcześniej działałem dobrze.” (Presses „revert” -> facet, który cofnął, odszedł na zawsze lub nie będzie dostępny w ciągu najbliższych tygodni, a następny cesjonariusz musi dowiedzieć się żmudnie jak ja, o co chodzi w tych plikach)

Ale czy nie zauważasz, dlaczego te pliki zostały usunięte z komunikatów zatwierdzania?

Oczywiście, że tak, ale komunikat zatwierdzenia czasami nie jest czytany przez kolegów. Nie jest typową sytuacją, że kiedy próbujesz zrozumieć kod (w moim przypadku martwy), najpierw sprawdzasz dziennik kontroli wersji ze wszystkimi powiązanymi komunikatami zatwierdzania. Zamiast przeszukiwać dziennik, kolega od razu widzi, że ten plik jest bezużyteczny. Oszczędza to jego czas i ona / on wie, że ten plik został prawdopodobnie przywrócony na złe (a przynajmniej rodzi pytanie.

version-control svn evidence-based

— Bruder Lustig
źródło

120

Zasadniczo próbujesz powielić zadanie systemu kontroli wersji bezpośrednio w pliku. Robienie czegoś takiego zdecydowanie podniosłoby flagę w mojej głowie. Jeśli twoi koledzy nie czytają komunikatów zatwierdzenia i wskrzeszają plik, który został poprawnie usunięty i przejdzie przegląd kodu, na pewno coś jest nie tak w twoim zespole i jest to świetna okazja, aby nauczyć go lepiej.

— Vincent Savard

6

@GregBurghardt To wydaje się dobre pytanie o zły pomysł. Być może ludzie przegrywają głosowanie w oparciu o drugą część tego (kiedy, IMO, powinni głosować po raz pierwszy)?

— Ben Aaronson

13

„Następnym razem, gdy muszę zatwierdzić / sprawdzić coś w projekcie do kontroli wersji, naciskam SVN-> Usuń” Czy mówisz, że usuwasz je w (potencjalnie) całkowicie niezwiązanym zatwierdzeniu?

— Kevin

21

Oczywiście, że tak, ale komunikat zatwierdzenia czasami nie jest czytany przez kolegów. - jeśli nie sprawdzają komunikatu zatwierdzenia, można bezpiecznie założyć, że nie są zainteresowani tym, dlaczego plik został usunięty ... w przeciwnym razie powinni sprawdzić komunikat zatwierdzenia.

— Ant P

9

Jeśli chcę wiedzieć, dlaczego plik zniknął z mojego VCS kasie, będę szukać w historii zmian pierwszy , a jeśli to nie wyjaśnia sprawy, będę czytać dyskusję, co wydarzyło się, gdy delecja recenzenta. (Jeśli nie masz proces weryfikacji kodu, że każda zmiana przechodzi przez, masz większe problemy.) A jeśli to jest wciąż unenlightening będę rozmawiać z kimkolwiek skasowany plik. Mogę spojrzeć na poprzednią zawartość pliku, aby spróbować dowiedzieć się, co kiedyś robił, ale nie w celu wyjaśnienia usunięcia.

— zwol

110

Problem z dodaniem komentarza do pliku, który powinien zostać usunięty, zamiast usunięcia go w kontroli źródła i umieszczenia tam wyjaśnienia, polega na założeniu, że jeśli programiści nie będą czytać komunikatów zatwierdzenia, z pewnością przeczytają komentarze w kodzie źródłowym.

Z perspektywy osoby trzeciej ta metodologia wydaje się być oparta na bardzo konserwatywnym podejściu do kontroli źródła.

„Co się stanie, jeśli usunę ten nieużywany plik, a następnie ktoś będzie go potrzebował?” ktoś może zapytać.

Używasz kontroli źródła. Cofnij zmianę lub lepiej porozmawiaj z osobą, która usunęła plik (komunikuj się).

„Co się stanie, jeśli usunę martwy plik, a następnie ktoś ponownie go użyje i wprowadzi zmiany?” ktoś inny może zapytać.

Ponownie używasz kontroli źródła. Dostaniesz konflikt scalania, który dana osoba musi rozwiązać. Odpowiedzią tutaj, podobnie jak w przypadku ostatniego pytania, jest komunikacja z członkami drużyny.

Jeśli naprawdę masz wątpliwości, że plik powinien zostać usunięty, komunikuj się przed usunięciem go z kontroli źródła. Być może dopiero niedawno przestał być używany, ale nadchodząca funkcja może tego wymagać. Nie wiesz tego, ale jeden z innych programistów może.

Jeśli należy go usunąć, usuń go. Wytnij tłuszcz z bazy kodu.

Jeśli zrobiłeś „oopsie” i rzeczywiście potrzebujesz pliku, pamiętaj, że używasz kontroli źródła, aby odzyskać plik.

Vincent Savard w komentarzu do pytania powiedział:

... Jeśli twoi koledzy nie czytają komunikatów zatwierdzania i wskrzeszają plik, który został prawidłowo usunięty i przejdzie przegląd kodu, na pewno coś jest nie tak w twoim zespole i jest to świetna okazja, aby nauczyć go lepiej.

To dobra rada. Przeglądy kodu powinny być tego rodzaju. Programiści muszą sprawdzać komunikaty zatwierdzania, gdy plik zostanie nieoczekiwanie zmieniony lub plik zostanie usunięty lub jego nazwa zostanie zmieniona.

Jeśli komunikaty zatwierdzania nie opowiadają historii, programiści muszą również pisać lepsze komunikaty zatwierdzania.

Obawa przed usunięciem kodu lub plików oznacza głębszy, systemowy problem z procesem:

Brak dokładnych recenzji kodu
Brak zrozumienia, jak działa kontrola źródła
Brak komunikacji w zespole
Słabe komunikaty zatwierdzania przez programistów

Są to problemy do rozwiązania, więc nie czujesz się, jakbyś rzucał kamieniami w szklany dom, gdy usuwasz kod lub pliki.

— Greg Burghardt
źródło

3

„założenie, że jeśli programiści nie będą czytać komunikatów zatwierdzania, z pewnością przeczytają komentarze w kodzie źródłowym”. Myślę, że jest to dość dobre założenie. Deweloper zmieniający plik musi faktycznie spojrzeć na plik, aby wykonać zadanie, a nic nie zmusza do spojrzenia na historię kontroli źródła.

— AShelly

7

@AShelly: zadaniem programisty rzadko jest zmiana komentarza. Zadania polegają na zmianie kodu, na czym koncentruje się deweloper - a nie na komentarzach.

— Greg Burghardt

21

@AShelly To, że muszą otworzyć plik, nie oznacza, że przeczytają określony komentarz na górze. Docelowi odbiorcy tej zmiany to najbardziej nieświadomy rodzaj programisty, który uważa, że ich potrzeby są jedynymi potrzebami i wszystko powinno się wokół nich kręcić. Nie są w stanie odczytać Twojego grzecznego komentarza. Co gorsza, prawdopodobnie przeczytają go, a następnie powrócą do następnej najstarszej wersji.

— Cort Ammon

5

@AShelly - jako przykład często otwieram pliki w lokalizacji. W VS mogę otworzyć bezpośrednio na metodę, korzystając z menu u góry lub menu kontekstowego „przejdź do definicji”. Nigdy nie zobaczyłem początku pliku. Również w Sublime Text często otwieram na linię Ich otwarte okna dialogowe users.rb: 123 otworzy users.rb bezpośrednio na linię 123. Wiele edytorów kodu ma bardzo podobne opcje.

— coteyr

2

Oprócz powyższego, im bardziej skomplikowane jest wykonywanie takich zadań czyszczenia, tym mniej prawdopodobne jest, że ludzie będą je regularnie wykonywać. Jeśli możesz to zrobić łatwiej, obniża to „energię aktywacji” dla ciebie i wszystkich innych. Jest to szczególnie ważne, jeśli próbujesz zachęcić innych członków zespołu do zmiany praktyki. Im bardziej złożona jest procedura, tym trudniej jest uzyskać wpisowe.

— xdhmoore

105

Tak, to zła praktyka.

Wyjaśnienie dotyczące usunięcia należy umieścić w komunikacie zatwierdzenia podczas zatwierdzania usunięcia plików.

Komentarze w plikach źródłowych powinny wyjaśniać kod, jak obecnie wygląda . Komunikaty zatwierdzania powinny wyjaśniać, dlaczego wprowadzono zmiany w zatwierdzeniu, więc historia zatwierdzeń w pliku wyjaśnia jego historię.

Pisanie komentarzy wyjaśniających zmiany bezpośrednio w samym źródle znacznie utrudni śledzenie kodu. Pomyśl o tym: jeśli komentujesz w pliku za każdym razem, gdy zmieniasz lub usuwasz kod, wkrótce pliki zostaną zalane komentarzami na temat historii zmian.

— JacquesB
źródło

6

Być może dodaj odpowiedź do pytania: Czy to zła praktyka? Tak, ponieważ ....

— Juan Carlos Coto

1

Czasami robię to samo (jak OP), ponieważ łatwiej jest odkomentować niż cofnąć. W rzadkich przypadkach, nawet jeśli kod kompiluje się i przechodzi automatyczne testowanie, istnieje powód istnienia tego kodu, którego nie zauważyłem, że wykryłby go tester ręczny. W tym rzadkim scenariuszu, zamiast przechodzić przez poważny ból lub cofać kilka zmian później, po prostu cofam komentarz do kodu (i wracam do tego, jak można go poprawić)

— Andrew Savinykh

2

@AndrewSavinykh To jest inaczej, komentujesz kod, którego nadal nie jesteś pewien, czy chcesz usunąć.

— Goyo,

6

@AndrewSavinykh „poważny ból lub cofnięcie kilku późniejszych zobowiązań” - zastanawiam się, jakiej kontroli wersji używasz; to nie powinno być poważnym bólem. Na pewno nie ma go w Git, przynajmniej po tym, jak zrobiłeś to kilka razy i nauczyłeś się, na co należy uważać ... i pod warunkiem, że twoja historia nie jest wypełniona niepotrzebnymi zmianami, które dają ci koliduje z każdym innym połączeniem. I zobowiązania, które jedynie komentują coś, są raczej skłonne to zrobić ...

— leftaroundabout

4

@AndrewSavinykh tak, i to nie tak, że nie spotkałem się z tymi problemami - projekty, których opiekunowie nigdy tak naprawdę nie pracowali z kontrolą wersji i umieścili wiele informacji w komentarzach, które powinny były być komunikatami o zatwierdzeniu, zamiast tego dodali zamiast tego nowe ręczne cofanie zatwierdzeń prawidłowego przywracania itp. Wynikowy stan repozytorium zapewnił wiele frajdy podczas konfliktów w każdym większym obszarze bazowym ... Ale i o to mi chodzi, te problemy są często w dużej mierze spowodowane przez obejścia, takie jak ten, którego tu bronisz.

— leftaroundabout

15

Moim zdaniem obie opcje nie są najlepszą praktyką , w przeciwieństwie do złej praktyki:

Dodanie komentarzy dodaje wartość tylko wtedy, gdy ktoś czyta te komentarze.
Samo usunięcie z VCS (z podaniem przyczyny w opisie zmiany) może mieć wpływ na krytyczne wyniki i wiele osób nie czyta opisu zmiany, szczególnie pod presją.

Moją ulubioną praktyką - głównie ze względu na to, że byłam gryziona kilka razy przez lata, pamiętając, że nie zawsze wiesz, gdzie lub przez kogo jest używany Twój kod - jest:

Dodaj komentarz stwierdzający, że jest to kandydat do usunięcia i wycofanie #warning lub podobne (większość języków ma jakiś taki mechanizm, np. W Javie , w najgorszym przypadku wystarczą printstwierdzenia lub podobne), najlepiej z pewnym przedziałem czasowym i z danymi kontaktowymi. Zaalarmuje to każdego, kto nadal używa kodu. Te ostrzeżenia są zwykle wewnątrz każdej funkcji lub klasy, jeśli takie rzeczy istnieją .
Po pewnym czasie zaktualizuj ostrzeżenie do standardowego zakresu plików #warning(niektóre osoby ignorują ostrzeżenia o wycofaniu, a niektóre łańcuchy narzędzi domyślnie nie wyświetlają takich ostrzeżeń).
Później zamień zakres pliku #warningna #errorlub równoważny - spowoduje to uszkodzenie kodu w czasie kompilacji, ale można, jeśli jest to absolutnie konieczne, usunąć, ale osoba go usuwająca nie może go zignorować. (Niektórzy programiści nie czytają ani nie reagują na żadne ostrzeżenia lub mają ich tak wiele, że nie są w stanie oddzielić ważnych).
Na koniec zaznacz plik (i) (w terminie lub po terminie) jako usunięte w VCS.
Jeśli usuwam cały pakiet / bibliotekę / etc na etapie ostrzegania, dodam plik README.txt lub README.html lub podobny z informacją o tym, kiedy i dlaczego planowane jest pozbycie się pakietu i pozostawienie tylko tego pliku, z zmień, aby powiedzieć, kiedy został usunięty, jako jedyna zawartość pakietu przez pewien czas po usunięciu reszty zawartości.

Zaletą tego podejścia jest to, że niektóre systemy kontroli wersji (CVS, PVCS itp.) Nie usuwają istniejącego pliku przy kasie, jeśli został on usunięty w VCS. Daje to również sumiennym programistom, tym, którzy naprawiają wszystkie swoje ostrzeżenia, mnóstwo czasu na rozwiązanie problemu lub odwołanie się od usunięcia. Zmusza to również pozostałych programistów do przynajmniej spojrzenia na powiadomienie o usunięciu i narzekania .

Zauważ, że #warning/ #errorjest mechanizmem C / C ++, który powoduje ostrzeżenie / błąd, a po nim dostarczony tekst, gdy kompilator napotka ten kod, java / java-doc ma @Deprecatedadnotację, aby wydać ostrzeżenie o użyciu i @deprecatedpodać pewne informacje itp. Istnieją przepisy na podobne działania w Pythonie i widziałem assertużywane do dostarczania podobnych informacji do #errorprawdziwego świata.

— Steve Barnes
źródło

7

W szczególności, ponieważ w tym pytaniu jest mowa o Javie, użyj @deprecatedkomentarza JavaDoc i @Deprecatedadnotacji .

— 200_success

8

Wydaje się to bardziej odpowiednie, gdy chcesz pozbyć się czegoś wciąż używanego. Ostrzeżenie może pozostać do momentu, aż znikną wszystkie zastosowania, ale gdy kod się skończy, należy go natychmiast usunąć.

— Andy,

6

@Andy Jednym z problemów z kodem typu biblioteki, szczególnie w Open Source lub w dużych organizacjach, jest to, że możesz myśleć, że kod jest martwy, ale odkrywasz, że jest on aktywny w jednym lub kilku miejscach, o których osobiście nigdy nie wyobrażałeś sobie że był używany. Czasami kod musi zostać zabity, ponieważ jest naprawdę zły lub zawiera zagrożenia bezpieczeństwa, ale po prostu nie wiesz, czy i gdzie jest używany.

— Steve Barnes,

1

@ 200_sukces dzięki - moje tło w C / C ++ pokazano - dodane do odpowiedzi.

— Steve Barnes,

1

@ SteveBarnes Może, ale nie o to pyta OP. Sprawdził, że kod jest martwy.

— Andy

3

Tak, powiedziałbym, że to zła praktyka, ale nie dlatego, że jest w plikach, a nie w komunikacie zatwierdzenia. Problem polega na tym, że próbujesz dokonać zmiany bez komunikacji ze swoim zespołem.

Ilekroć wprowadzasz jakiekolwiek zmiany do pnia - dodając, usuwając lub modyfikując pliki w jakikolwiek sposób - powinieneś, zanim zwrócisz się do pnia, rozmawiać o tych zmianach bezpośrednio z członkiem (lub członkami zespołu), który byłby bezpośrednio posiadający wiedzę na ich temat (w zasadzie omawiajcie, co chcielibyście umieścić w tych nagłówkach bezpośrednio z kolegami z drużyny). Zapewni to, że (1) usuwany kod naprawdę musi zostać usunięty, a (2) Twój zespół będzie o wiele bardziej prawdopodobne, że dowie się, że kod został usunięty, a zatem nie spróbuje cofnąć usuniętych danych. Nie wspominając o tym, że otrzymasz także wykrywanie błędów i tym podobne dla dodanego kodu.

Oczywiście, kiedy zmieniasz duże rzeczy, powinieneś również umieścić je w komunikacie zatwierdzenia, ale ponieważ już to omawiałeś, nie musi to być źródło ważnych ogłoszeń. Odpowiedź Steve'a Barnesa dotyczy również kilku dobrych strategii, które można zastosować, jeśli potencjalna pula użytkowników dla twojego kodu jest zbyt duża (np. Użycie przestarzałych znaczników języka zamiast usuwania plików na początku).

Jeśli nie chcesz iść tak długo bez zobowiązania (tzn. Zmiana ma sens w przypadku kilku wersji), dobrym pomysłem jest utworzenie gałęzi z pnia i zatwierdzenie do gałęzi, a następnie scalenie gałęzi z powrotem w pień, gdy zmiana jest gotowa. W ten sposób VCS nadal tworzy kopię zapasową pliku, ale twoje zmiany w żaden sposób nie wpływają na łącze.

— TheHansinator
źródło

1

Powiązany problem związany z projektami opartymi na wielu platformach i konfiguracjach. Tylko dlatego, że ustalenie, że jest martwy, nie oznacza, że jest to właściwe określenie. Pamiętaj, że martwy w użyciu może nadal oznaczać szczątkową zależność, którą wciąż trzeba usunąć, a niektóre mogły zostać pominięte.

Więc (w projekcie C ++) dodałem

#error this is not as dead as I thought it was

I sprawdziłem to. Poczekaj, aż przejdzie przez odbudowę wszystkich normalnych platform i nikt nie narzeka. Gdyby ktoś go znalazł, łatwo byłoby usunąć jedną linię, zamiast oszołomić brakującym plikiem.

Zgadzam się co do zasady, że wspomniane komentarze duplikują funkcję, którą należy wykonać w systemie zarządzania wersjami . Ale możesz mieć konkretne powody, aby je uzupełnić. Brak znajomości narzędzia VCS nie jest jednym z nich.

— JDługosz
źródło

-1

Na kontinuum złych praktyk jest to dość niewielkie. Zrobię to czasami, gdy chcę sprawdzić oddział i zobaczyć stary kod. Może to ułatwić porównanie z kodem zastępczym. Zazwyczaj pojawia się komentarz do recenzji kodu „cruft”. Z małym wyjaśnieniem przechodzę przegląd kodu.

Ale ten kod nie powinien długo żyć. Generalnie usuwam na początku następnego sprintu.

Jeśli masz współpracowników, którzy nie czytają komunikatów zatwierdzania lub nie pytają cię, dlaczego zostawiłeś kod w projekcie, problem nie dotyczy złego stylu kodowania. Masz inny problem.

— Tony Ennis
źródło

wydaje się, że nie oferuje to nic istotnego w porównaniu z punktami przedstawionymi i wyjaśnionymi w poprzednich 6 odpowiedziach

— komnata

-3

możesz również zmienić klasę i zmienić jej nazwę z prefiksem UNUSED_Classname, zanim wykonasz wyczyn. Następnie powinieneś bezpośrednio zatwierdzić operację usunięcia

Krok 1: zmień nazwę klasy, dodaj swój komentarz, zatwierdź
Krok 2: Usuń plik i zatwierdź

Jeśli jest to martwy kod, usuń go. Każdy tego potrzebuje, może wrócić, ale krok zmiany nazwy uniemożliwi bezpośrednie użycie go bez zastanowienia.

Naucz także ludzi, jak sprawdzać wszystkie komunikaty zatwierdzania pliku, niektórzy nawet nie wiedzą, że jest to możliwe.

— użytkownik275398
źródło

6

„Refaktoryzacja” nie znaczy tak naprawdę, jak myślisz

— Nik Kyriakides,

6

Nie trzeba zmieniać nazwy klasy / metody, aby upewnić się, że jest nieużywana, usunięcie również działa równie dobrze. Zamiast tego procedura jest taka sama, jak każdej innej zmiany: 1) usuń martwy kod , 2) uruchom testy , 3) jeśli przejdą, zatwierdź z komentarzem .

— Schwern,

1

@ user275398 Myślę też, że zmiana nazw plików to naprawdę za dużo. A także z technologicznego punktu widzenia, SVN traktuje zmianę nazwy tak, jakby plik został usunięty i utworzony z nową nazwą, dzięki czemu masz przerwę w historii. Jeśli przywrócisz plik o zmienionej nazwie i przejrzysz jego dziennik SVN, historia przed zmianą nazwy nie będzie dostępna. W tym celu musisz przywrócić plik o starej nazwie. Refaktoryzacja jest przy okazji czymś innym, refaktoryzacja organizuje istniejący kod w nową strukturę.

— Bruder Lustig,

@BruderLustig: Dobre oprogramowanie na pewno tego nie zrobi. Git ma git mv, który śledzi historię. Podobnie Mercurial hg mv. Wygląda na to, że svn movepowinien również działać dla SVN?

— wchargin

@wchargin Nie, git mvpo prostu przenosi plik i etapy usuwania pliku i tworzenia pliku. Całe śledzenie ruchu ma miejsce, gdy biegniesz git log --followi podobnie. gitnie ma możliwości śledzenia nazw, w rzeczywistości w ogóle nie śledzi zmian ; zamiast tego śledzi stany w momencie zatwierdzenia i dowiaduje się, co zmieniło się w czasie, gdy przeglądasz historię.

— maaartinus