Pokrycie kodu podkreśla nieużywane metody - co powinienem zrobić?

59

Zadanie polegało na zwiększeniu zasięgu kodu istniejącego projektu Java.

Zauważyłem, że narzędzie do pokrycia kodu ( EclEmma ) wyróżniło niektóre metody, które nigdy nie są wywoływane z dowolnego miejsca.

Moja początkowa reakcja nie polega na pisaniu testów jednostkowych dla tych metod, ale na ich podkreśleniu mojemu przełożonemu / zespołowi i zapytaniu, dlaczego te funkcje są na początek.

Jakie byłoby najlepsze podejście? Napisz dla nich testy jednostkowe lub zadaj pytanie, dlaczego tam są?

Lucas T.
źródło
1
Komentarze nie są przeznaczone do rozszerzonej dyskusji; ta rozmowa została przeniesiona do czatu .
wałek klonowy

Odpowiedzi:

119
  1. Usunąć.
  2. Popełnić.
  3. Zapomnieć.

Racjonalne uzasadnienie:

  1. Martwy kod jest martwy. Przez sam opis nie ma celu. W pewnym momencie mógł mieć cel, ale go nie ma, więc kod powinien zniknąć.
  2. Kontrola wersji zapewnia, że ​​w (moim doświadczeniu) rzadkim wyjątkowym zdarzeniu, które ktoś przyjdzie później w poszukiwaniu tego kodu, może zostać odzyskane.
  3. Jako efekt uboczny natychmiast poprawiasz zasięg kodu, nie robiąc nic (chyba że testowany jest martwy kod, co rzadko się zdarza).

Zastrzeżenie z komentarzy: Oryginalna odpowiedź zakładała, że ​​dokładnie sprawdziłeś, czy kod jest niewątpliwie martwy. IDE są omylne i istnieje kilka sposobów na wywołanie martwego kodu. Sprowadź eksperta, chyba że masz absolutną pewność.

l0b0
źródło
118
Generalnie zgodziłbym się z tym podejściem, ale jeśli jesteś nowy w projekcie i / lub młodszy programista, lepiej zapytaj swojego mentora / przełożonego przed usunięciem. W zależności od projektu niektóre metody mogą nie wyglądać na używane, ale mogą być wywoływane / używane przez kod zewnętrzny spoza kodu projektu, do którego masz dostęp. Może to być kod generowany automatycznie, więc usunięcie nie spowoduje nic poza szumem historii dziennika. Twoje IDE może nie być w stanie znaleźć dostępu opartego na odbiciu z poziomu projektu. Itp. Pp. Jeśli nie masz pewności co do takich przypadków narożnych, zapytaj przynajmniej raz.
Frank Hopkins
11
Chociaż zgadzam się z rdzeniem tej odpowiedzi, dosłowne pytanie brzmiało: „czy mam zadawać pytanie, dlaczego one istnieją?”. . Ta odpowiedź może sprawiać wrażenie, że OP nie powinien pytać swojego zespołu przed usunięciem.
Doc Brown
58
Najpierw jednak dodam krok - sprawdź dziennik winy git pod kątem nieużywanych linii kodu. Jeśli znajdziesz osobę, która je napisała, możesz zapytać, po co jest. Jeśli linie są nowe, istnieje duża szansa, że ​​ktoś planuje ich wkrótce użyć (w takim przypadku prawdopodobnie powinien być teraz przetestowany jednostkowo).
bdsl
10
Z tą odpowiedzią, wyrażoną w komentarzach lub innych odpowiedziach, jest tyle błędów, że nie mogę uwierzyć, że jest to najczęściej głosowana i zaakceptowana odpowiedź.
user949300
11
@Emory Najlepszym sposobem na rozpoczęcie pracy w nowym zespole jest przełamanie kompilacji poprzez nieostrożne usuwanie rzeczy, ponieważ uważałeś, że nikt ich nie potrzebuje. Gwarantuje, że jesteś popularny od pierwszego dnia. Pewnie może nie być potrzebny (ponieważ każda duża, starsza aplikacja zawsze ma kaszel obejmujący 100% kodu ), ale to bardzo zły zwrot z inwestycji.
Voo
55

Wszystkie pozostałe odpowiedzi oparte są na założeniu, że omawiane metody są naprawdę nieużywane. Jednak pytanie nie określało, czy ten projekt jest samowystarczalny, czy jakaś biblioteka.

Jeśli dany projekt jest biblioteką, pozornie nieużywane metody można zastosować poza projektem, a ich usunięcie spowodowałoby uszkodzenie innych projektów. Jeśli sama biblioteka jest sprzedawana klientom lub udostępniana publicznie, wyśledzenie użycia tych metod może być nawet niemożliwe.

W takim przypadku istnieją cztery możliwości:

  • Jeśli metody są prywatne lub prywatne, można je bezpiecznie usunąć.
  • Jeśli metody są publiczne, ich obecność może być uzasadniona, nawet bez faktycznego użycia, dla kompletności funkcji. Powinny być jednak przetestowane.
  • Jeśli metody są publiczne i niepotrzebne, ich usunięcie będzie przełomową zmianą, a jeśli biblioteka będzie przestrzegać wersji semantycznej , jest to dozwolone tylko w nowej wersji głównej.
  • Alternatywnie metody publiczne mogą być również przestarzałe i usunięte później. Daje to trochę czasu użytkownikom interfejsu API na przejście z przestarzałych funkcji, zanim zostaną usunięte w następnej głównej wersji.
Zoltan
źródło
9
Dodatkowo, jeśli jest to biblioteka, funkcje są dostępne ze względu na kompletność
PlasmaHH
Czy potrafisz opracować sposób ich przetestowania? Jeśli nie wiesz, dlaczego ta metoda istnieje, prawdopodobnie nie wiesz, co powinna zrobić.
TKK
Nazwy metod, takich jak filter_name_exists, ReloadSettingslub addToSchema(losowo wybranych z 3 dowolnych projektów Open Source) powinna dostarczyć kilka wskazówek na co metoda ma to zrobić. Komentarz javadoc może być jeszcze bardziej przydatny. Wiem, że nie jest to właściwa specyfikacja, ale może wystarczyć do stworzenia kilku testów, które mogą przynajmniej zapobiec regresjom.
Zoltan
1
publiczne metody w prywatnej klasie lub interfejsie nie powinny być w tym celu uważane za publiczne. podobnie, jeśli klasa publiczna jest zagnieżdżona w klasie prywatnej, nie jest tak naprawdę publiczna.
emory
31

Najpierw sprawdź, czy narzędzie do pokrywania kodu jest prawidłowe.

Miałem sytuacje, w których nie wykryły metod wywoływanych przez odwołania do interfejsu lub gdy klasa jest gdzieś dynamicznie ładowana.

Ewan
źródło
Dzięki, zrobię. W Eclipse szukam funkcji na podstawie kodu i nic się nie pojawia. Jeśli masz jakieś sugestie dotyczące bardziej kompleksowego wyszukiwania, byłbym bardzo wdzięczny.
Lucas T
3
tak, powinieneś sprawdzić inne projekty, które mogłyby zaimportować klasę jako dll
Ewan
7
Ta odpowiedź jest niepełna. „Najpierw sprawdź, czy twoje narzędzie do pokrycia kodu jest prawidłowe. Jeśli tak, to… [wstaw resztę odpowiedzi] ”. W szczególności OP chce wiedzieć, co zrobić, jeśli narzędzie do pokrywania kodu jest prawidłowe.
Jon Bentley
1
@jonbentley OP prosi o najlepsze podejście do raportu o narzędziach. „Sprawdź to ręcznie”, ponieważ z kontekstu wynika, że ​​jest niepoprawny
Ewan
15

Ponieważ Java jest kompilowana statycznie, usuwanie metod powinno być całkiem bezpieczne. Usuwanie martwego kodu jest zawsze dobre. Istnieje pewne prawdopodobieństwo, że istnieje jakiś szalony system refleksji, który uruchamia je w czasie wykonywania, więc najpierw sprawdź z innymi programistami, ale w przeciwnym razie usuń je.

max630
źródło
9
+1 za sprawdzanie z ludźmi, to rodzaj zasady najmniejszej niespodzianki. Nie chcesz, aby ktoś spędzał zbyt dużo czasu na szukaniu metody, którą właśnie zostawiłem kilka zmian wcześniej. Ponadto, w niektórych przypadkach martwy kod może być nowym elementem, który jest już zarejestrowany, ale nigdzie jeszcze nie podłączony (chociaż w tym przypadku powinien być dobrze udokumentowany komentarzami i przetestowany).
Frax
4
No chyba, że ​​kod używa refleksji do wywoływania „nieużywanych” metod, ale w takim przypadku masz o wiele większe problemy, a my czekamy na kod na stronie thefailywft.com (Wykonaj szybki test, aby sprawdzić, czy jakiś kod ma nazwę klasy. klasa).
MTilsted
2
Jest statycznie skompilowany, ale jest też invokedynamicinstrukcja, więc wiesz ...
corsiKa
@MTilsted: Istnieje wiele frameworków, które będą wywoływały kod za pomocą ciągów - przynajmniej mogę wymyślić Spring i Hibernację z góry mojej głowy.
jhominal
9

Jakie byłoby najlepsze podejście? Napisz dla nich testy jednostkowe lub zadaj pytanie, dlaczego tam są?

Usunięcie kodu to dobra rzecz.

Jeśli nie możesz usunąć kodu, możesz z pewnością oznaczyć go jako @Deprecated , dokumentując, do którego głównego wydania chcesz usunąć metodę. Następnie możesz go usunąć „później”. W międzyczasie będzie jasne, że nie należy dodawać nowego kodu, który będzie od niego zależny.

Nie zalecałbym inwestowania w przestarzałe metody - dlatego nie ma nowych testów jednostkowych, które mogłyby tylko osiągnąć cele dotyczące zasięgu.

Różnica między nimi polega przede wszystkim na tym, czy metody są częścią opublikowanego interfejsu . Arbitralne usuwanie części opublikowanego interfejsu może być nieprzyjemną niespodzianką dla konsumentów zależnych od interfejsu.

Nie mogę rozmawiać z EclEmma, ​​ale z moich doświadczeń wynika, że ​​jedną z rzeczy, na które należy uważać, jest refleksja. Jeśli na przykład użyjesz tekstowych plików konfiguracyjnych, aby wybrać klasy / metody dostępu, użyte / nieużywane rozróżnienie może nie być oczywiste (zostałem przez to wypalony kilka razy).

Jeśli twój projekt jest liściem na wykresie zależności, wówczas argument dotyczący wycofania jest osłabiony. Jeśli twój projekt jest biblioteką, to argument za zaniechaniem jest silniejszy.

Jeśli Twoja firma korzysta z mono-repo , wówczas usuwanie jest mniejsze ryzyko niż w przypadku wielu repo.

Jak zauważono w l0b0 , jeśli metody są już dostępne w kontroli źródła, odzyskanie ich po usunięciu jest prostym ćwiczeniem. Jeśli naprawdę martwiłeś się koniecznością zrobienia tego, zastanów się, jak zorganizować swoje zatwierdzenia, aby w razie potrzeby móc odzyskać usunięte zmiany.

Jeśli niepewność jest wystarczająco wysoka, możesz rozważyć skomentowanie kodu , zamiast go usunąć. To dodatkowa praca na szczęśliwej ścieżce (gdzie usunięty kod nigdy nie jest przywracany), ale ułatwia przywracanie. Domyślam się, że powinieneś preferować proste usuwanie, dopóki nie zostaniesz przez to spalony kilka razy, co da ci wgląd w to, jak ocenić „niepewność” w tym kontekście.

pytanie, dlaczego tam są?

Czas zainwestowany w zdobywanie wiedzy niekoniecznie jest marnowany. Przeprowadzono usuwanie w dwóch krokach - najpierw poprzez dodanie i zatwierdzenie komentarza wyjaśniającego, czego dowiedzieliśmy się o kodzie, a następnie usunięcie kodu (i komentarza).

Możesz także użyć czegoś analogicznego do architektonicznych zapisów decyzji jako sposobu przechwytywania wiedzy za pomocą kodu źródłowego.

VoiceOfUnreason
źródło
9

Narzędzie do pokrywania kodu nie jest wszechwiedzące, wszechwidzące. To, że twoje narzędzie twierdzi, że metoda nie jest wywoływana, nie oznacza, że nie jest wywoływana. Jest refleksja, a w zależności od języka mogą istnieć inne sposoby wywołania metody. W C lub C ++ mogą występować makra konstruujące nazwy funkcji lub metod, a narzędzie może nie widzieć wywołania. Tak więc pierwszym krokiem byłoby: Wykonaj wyszukiwanie tekstowe nazwy metody i powiązanych nazw. Zapytaj doświadczonych kolegów. Może się okazać, że jest faktycznie używany.

Jeśli nie jesteś pewien, umieść aser () na początku każdej „nieużywanej” metody. Może to się nazywa. Lub oświadczenie logowania.

Może kod jest naprawdę cenny. Może to być nowy kod, nad którym kolega pracuje od dwóch tygodni i że jutro go włączy. Nie nazywa się go dzisiaj, ponieważ wezwanie do niego zostanie dodane jutro.

Może kod jest w rzeczywistości cenny, część 2: Kod może przeprowadzać bardzo drogie testy uruchomieniowe, które mogłyby wykryć, że coś pójdzie nie tak. Kod jest włączany tylko wtedy, gdy coś pójdzie nie tak. Być może usuwasz cenne narzędzie do debugowania.

Co ciekawe, najgorsza możliwa rada „Usuń. Zaakceptuj. Zapomnij”. jest najwyżej oceniony. (Recenzje kodu? Nie robisz recenzji kodu? Co do diabła robisz programując, jeśli nie robisz recenzji kodu?)

gnasher729
źródło
Z szacunkiem nie zgadzam się, że „DELETE. COMMIT. FORGET” to najgorsza rada. (Myślę, że to jest najlepsze.). Twoje podejście jest również OK. Myślę, że najgorszą możliwą radą byłoby napisanie testów jednostkowych, które wykorzystują „martwy kod”, ale nie dają żadnych zapewnień. Narzędzie do pokrywania kodu zostanie oszukane, myśląc, że są używane.
emory
3
Nic w odpowiedzi „Usuń. Zatwierdź. Zapomnij” nie mówi, aby nie sprawdzać kodu. Jest całkowicie możliwe (i wskazane, IMHO) sprawdzenie kodu po jego zatwierdzeniu (ale przed wdrożeniem :-)).
śleske
@sleske Jak oceniasz kod, którego już nie ma? Ta odpowiedź nie wspomina również o „przeglądzie”.
user949300
@ user949300: To, czy zmiana kodu wymaga przeglądu, czy nie, to osobne pytanie, niezależnie od tego, czy kod zostanie dodany, zmieniony czy usunięty (dodanie kodu może być jeszcze bardziej katastrofalne niż usunięcie go, patrz np. luka Heartbleed). Dodatkowo, odpowiedź (teraz) mówi „sprowadzić eksperta”, co brzmi dość blisko recenzji kodu.
śleske
1
@ user949300: Co do „Jak oceniasz kod, którego już nie ma” - Mam nadzieję, że to oczywiste: patrząc na zmianę wybranego narzędzia kontroli wersji. Jak inaczej sprawdziłbyś zmiany (jakiekolwiek zmiany)?
śleske
6

W zależności od środowiska, w którym działa oprogramowanie, możesz się zalogować, jeśli metoda zostanie kiedykolwiek wywołana. Jeśli nie zostanie wywołany w odpowiednim czasie, metodę można bezpiecznie usunąć.

Jest to bardziej ostrożne podejście niż tylko usunięcie metody i może być przydatne, jeśli pracujesz w środowisku wrażliwym na awarie.

Logujemy się do dedykowanego #unreachable-codekanału luzu z unikalnym identyfikatorem dla każdego kandydata do usunięcia i okazało się to dość skuteczne.

Jamie Bull
źródło
Ale co, jeśli rzadko się nazywa (na przykład średnio 1 na 1440) ?
Peter Mortensen
wtedy „odpowiedni okres czasu” jest dłuższy (chociaż lubię „mężczyznę po północy”)
Jamie Bull