Die Codeabdeckung zeigt unbenutzte Methoden auf - was soll ich tun?

59

Ich wurde beauftragt, die Codeabdeckung eines vorhandenen Java-Projekts zu erhöhen.

Ich habe festgestellt, dass das Code Coverage Tool ( EclEmma ) einige Methoden hervorgehoben hat, die niemals von irgendwoher aufgerufen werden.

Meine erste Reaktion ist nicht , Unit-Tests für diese Methoden zu schreiben, sondern sie meinem Vorgesetzten / Team vorzustellen und zu fragen, warum diese Funktionen überhaupt vorhanden sind.

Was wäre der beste Ansatz? Schreibe Unit-Tests für sie oder frage dich, warum sie da sind?

Lucas T
quelle
1
Kommentare sind nicht für längere Diskussionen gedacht. Diese Unterhaltung wurde in den Chat verschoben .
maple_shaft

Antworten:

119
  1. Löschen.
  2. Verpflichten.
  3. Vergessen.

Begründung:

  1. Toter Code ist tot. Schon nach seiner Beschreibung hat es keinen Zweck. Vielleicht hatte es irgendwann einen Zweck, aber der ist weg, und der Code sollte weg sein.
  2. Die Versionskontrolle stellt sicher, dass in dem (meiner Erfahrung nach) seltenen, einzigartigen Fall, dass jemand später nach diesem Code sucht, dieser abgerufen werden kann.
  3. Als Nebeneffekt verbessern Sie sofort die Codeabdeckung, ohne etwas zu tun (es sei denn, der tote Code wird getestet, was selten der Fall ist).

Einschränkung durch Kommentare: In der ursprünglichen Antwort wurde davon ausgegangen, dass Sie gründlich überprüft haben, dass der Code zweifelsohne tot ist. IDEs sind fehlbar, und es gibt verschiedene Möglichkeiten, wie Code, der tot aussieht, tatsächlich aufgerufen werden kann. Wenden Sie sich an einen Experten, es sei denn, Sie sind sich absolut sicher.

l0b0
quelle
118
Im Allgemeinen würde ich diesem Ansatz zustimmen, aber wenn Sie neu in einem Projekt und / oder einem Junior-Entwickler sind, fragen Sie besser Ihren Mentor / Vorgesetzten, bevor Sie es löschen. Je nach Projekt sehen einige Methoden möglicherweise nicht verwendet aus, werden jedoch von externem Code aufgerufen / verwendet, der nicht im Projektcode enthalten ist, auf den Sie Zugriff haben. Es kann sich um automatisch generierten Code handeln, sodass beim Entfernen nur Protokollgeräusche auftreten. Ihre IDE kann möglicherweise keinen reflexionsbasierten Zugriff innerhalb des Projekts finden. Usw. S. Wenn Sie sich über solche Eckfälle nicht sicher sind, fragen Sie mindestens einmal.
Frank Hopkins
11
Obwohl ich mit dem Kern dieser Antwort einverstanden bin, lautete die wörtliche Frage: "Soll ich fragen, warum sie da sind?" . Diese Antwort könnte den Eindruck erwecken, dass das OP sein Team vor dem Löschen nicht fragen sollte.
Doc Brown
58
Ich würde jedoch zuerst einen Schritt hinzufügen - überprüfen Sie das Git-Schuld-Protokoll für die nicht verwendeten Codezeilen. Wenn Sie die Person finden, die sie geschrieben hat, können Sie fragen, wozu sie da sind. Wenn die Leitungen neu sind, besteht eine gute Chance, dass sie bald von jemandem verwendet werden (in diesem Fall sollten sie jetzt wahrscheinlich einem Komponententest unterzogen werden).
BDSL
10
Es gibt so viele Dinge, die an dieser Antwort falsch sind, wie in Kommentaren oder anderen Antworten zum Ausdruck gebracht, dass ich nicht glauben kann, dass dies die am häufigsten gewählte und akzeptierte Antwort ist.
user949300
11
@Emory Der beste Weg, um in einem neuen Team zu beginnen, besteht darin, den Build zu brechen, indem Sie Dinge nachlässig löschen, weil Sie dachten, dass niemand sie benötigt. Garantiert, dass Sie ab dem ersten Tag beliebt sind. Sicher, dass dies möglicherweise nicht erforderlich ist (da jede große, ältere Anwendung immer zu 100% Code-Coverage- Husten aufweist ), aber das ist eine sehr schlechte Rendite.
Voo
55

Alle anderen Antworten basieren auf der Annahme, dass die fraglichen Methoden wirklich nicht verwendet werden. In der Frage wurde jedoch nicht angegeben, ob dieses Projekt eigenständig oder eine Bibliothek ist.

Wenn es sich bei dem betreffenden Projekt um eine Bibliothek handelt, können die anscheinend nicht verwendeten Methoden außerhalb des Projekts verwendet werden, und das Entfernen dieser Methoden würde die anderen Projekte beschädigen. Wenn die Bibliothek selbst an Kunden verkauft oder öffentlich zugänglich gemacht wird, kann es sogar unmöglich sein, die Verwendung dieser Methoden zu verfolgen.

In diesem Fall gibt es vier Möglichkeiten:

  • Wenn die Methoden privat oder paketprivat sind, können sie sicher entfernt werden.
  • Wenn die Methoden öffentlich sind, kann ihre Anwesenheit aus Gründen der Vollständigkeit der Funktionen auch ohne tatsächliche Verwendung gerechtfertigt sein. Sie sollten jedoch getestet werden.
  • Wenn die Methoden öffentlich sind und nicht benötigt werden, stellt das Entfernen eine grundlegende Änderung dar. Wenn die Bibliothek der semantischen Versionierung folgt , ist dies nur in einer neuen Hauptversion zulässig.
  • Alternativ können öffentliche Methoden auch später verworfen und entfernt werden. Dies gibt API-Verbrauchern einige Zeit, um von den veralteten Funktionen zu wechseln, bevor sie in der nächsten Hauptversion entfernt werden.
Zoltan
quelle
9
Plus, wenn es eine Bibliothek ist, sind die Funktionen der Vollständigkeit halber da
PlasmaHH
Können Sie erläutern, wie sie getestet werden sollten? Wenn Sie nicht wissen, warum die Methode vorhanden ist, wissen Sie wahrscheinlich nicht, was sie tun soll.
TKK
Methodennamen wie filter_name_exists, ReloadSettingsoder addToSchema(zufällig aus drei beliebigen Open - Source - Projekten gepflückt) sollten ein paar Hinweise geben , was das Verfahren es tun sollte. Ein Javadoc-Kommentar kann sogar noch nützlicher sein. Ich weiß, dass dies keine richtige Spezifikation ist, aber es könnte ausreichen, einige Tests zu erstellen, die zumindest Regressionen verhindern könnten.
Zoltan
1
Öffentliche Methoden für eine private Klasse oder Schnittstelle sollten zu diesem Zweck nicht als öffentlich betrachtet werden. Ebenso ist eine öffentliche Klasse, die in einer privaten Klasse verschachtelt ist, nicht wirklich öffentlich.
Emory
31

Überprüfen Sie zunächst, ob Ihr Code-Coverage-Tool korrekt ist.

Ich hatte Situationen, in denen sie nicht auf Methoden aufmerksam wurden, die über Verweise auf die Schnittstelle aufgerufen wurden, oder wenn die Klasse dynamisch irgendwo geladen wurde.

Ewan
quelle
Danke, werde tun. Bei Eclipse suche ich nach Funktionen auf Codebasis, und es wird nichts angezeigt. Wenn Sie weitere Vorschläge für eine umfassendere Suche haben, wäre ich Ihnen sehr dankbar.
Lucas T
3
ja, sie sollten andere projekte prüfen, die die klasse als dll importieren könnten
ewan
7
Diese Antwort fühlt sich unvollständig an. Msgstr "Überprüfen Sie zuerst, ob Ihr Code Coverage Tool korrekt ist. Wenn ja, dann ... [Rest der Antwort einfügen] ". Insbesondere möchte das OP wissen, was zu tun ist, wenn das Code-Coverage-Tool korrekt ist.
Jon Bentley
1
@jonbentley Das OP fragt nach dem besten Ansatz für den Tools-Bericht. "Überprüfen Sie es manuell", weil es aus dem Kontext ziemlich offensichtlich ist, dass es falsch ist
Ewan
15

Da Java statisch kompiliert ist, sollte es ziemlich sicher sein, die Methoden zu entfernen. Toten Code zu entfernen ist immer gut. Es besteht eine gewisse Wahrscheinlichkeit, dass es ein verrücktes Reflection-System gibt, das sie zur Laufzeit ausführt. Wenden Sie sich also zuerst an andere Entwickler, und entfernen Sie sie ansonsten.

max630
quelle
9
+1 für die Überprüfung mit Menschen, es ist sozusagen das Prinzip der geringsten Überraschung. Sie möchten nicht, dass jemand zu viel Zeit damit verbringt, nach der Methode zu suchen, die nur ein paar Mal zuvor verwendet wurde. In einigen Edge-Fällen kann toter Code das neue Material sein, das bereits eingecheckt, aber noch nirgendwo verkabelt ist (in diesem Fall sollte es jedoch mit Kommentaren gut dokumentiert und getestet werden).
Frax
4
Nun, es sei denn, der Code verwendet Reflection, um die "unbenutzten" Methoden aufzurufen, aber in diesem Fall haben Sie weitaus größere Probleme, und wir freuen uns darauf, den Code auf thefailywft.com zu sehen. Klasse).
MTMit
2
Es ist statisch kompiliert, aber es gibt auch die invokedynamicAnweisung, also weißt du ...
corsiKa
@MTilsted: Es gibt viele Frameworks, die Code mit Hilfe von Strings aufrufen. Ich kann mir mindestens Spring und Hibernate vorstellen.
29.
9

Was wäre der beste Ansatz? Schreibe Unit-Tests für sie oder frage dich, warum sie da sind?

Das Löschen von Code ist eine gute Sache.

Wenn Sie den Code nicht löschen können, können Sie ihn auf jeden Fall als @Deprecated markieren , um zu dokumentieren, auf welche Hauptversion Sie abzielen, um die Methode zu entfernen. Dann können Sie es "später" löschen. In der Zwischenzeit wird klar sein, dass kein neuer Code hinzugefügt werden sollte, der davon abhängt.

Ich würde nicht empfehlen, in veraltete Methoden zu investieren - also keine neuen Unit-Tests, um nur die Abdeckungsziele zu erreichen.

Der Unterschied zwischen den beiden besteht hauptsächlich darin, ob die Methoden Teil der veröffentlichten Schnittstelle sind oder nicht . Das willkürliche Löschen von Teilen der veröffentlichten Benutzeroberfläche kann für Benutzer, die von der Benutzeroberfläche abhängig waren, eine unangenehme Überraschung sein.

Ich kann nicht mit EclEmma sprechen, aber nach meinen Erfahrungen ist eines der Dinge, auf die Sie achten müssen, das Nachdenken. Wenn Sie zum Beispiel Textkonfigurationsdateien verwenden, um auszuwählen, auf welche Klassen / Methoden zugegriffen werden soll, ist die verwendete / nicht verwendete Unterscheidung möglicherweise nicht offensichtlich (ich bin dadurch ein paar Mal verbrannt worden).

Wenn es sich bei Ihrem Projekt um ein Blatt im Abhängigkeitsdiagramm handelt, wird der Fall für die Nichtbeachtung abgeschwächt. Wenn es sich bei Ihrem Projekt um eine Bibliothek handelt, ist die Begründung für die Nichtbeachtung strenger.

Wenn Ihr Unternehmen ein Mono-Repo verwendet , ist das Risiko beim Löschen geringer als im Multi-Repo-Fall.

Wenn die Methoden bereits in der Quellcodeverwaltung verfügbar sind, ist das Wiederherstellen nach dem Löschen, wie in l0b0 erwähnt , eine einfache Aufgabe . Wenn Sie dies wirklich befürchten, sollten Sie sich überlegen, wie Sie Ihre Commits so organisieren, dass Sie die gelöschten Änderungen bei Bedarf wiederherstellen können.

Wenn die Unsicherheit hoch genug ist, können Sie den Code auskommentieren, anstatt ihn zu löschen. Es ist zusätzliche Arbeit im Happy-Pfad (wo der gelöschte Code nie wiederhergestellt wird), aber es erleichtert die Wiederherstellung. Ich vermute, Sie sollten ein direktes Löschen bevorzugen, bis Sie ein paarmal davon verbrannt wurden, was Ihnen einige Einsichten darüber gibt, wie Sie "Unsicherheit" in diesem Kontext bewerten können.

Frage, warum sie da sind?

Zeit, die in das Erfassen der Überlieferungen investiert wird, ist nicht unbedingt verschwendet. Es ist bekannt, dass ich ein Entfernen in zwei Schritten durchführe - erstens indem ich einen Kommentar hinzufüge und festlege, in dem erklärt wird, was wir über den Code gelernt haben, und dann den Code (und den Kommentar) später lösche.

Sie können auch etwas verwenden, das mit Architekturentscheidungsunterlagen vergleichbar ist , um die Überlieferungen mit dem Quellcode zu erfassen.

VoiceOfUnreason
quelle
9

Ein Code-Coverage-Tool ist nicht allwissend, allsehend. Nur weil Ihr Tool angibt, dass die Methode nicht aufgerufen wurde, heißt das nicht, dass sie nicht aufgerufen wurde. Es findet eine Reflektion statt, und je nach Sprache kann die Methode auch auf andere Weise aufgerufen werden. In C oder C ++ gibt es möglicherweise Makros, die Funktions- oder Methodennamen konstruieren, und das Tool kann den Aufruf möglicherweise nicht sehen. Der erste Schritt wäre also: Führen Sie eine Textsuche nach dem Methodennamen und verwandten Namen durch. Fragen Sie erfahrene Kollegen. Sie können feststellen, dass es tatsächlich verwendet wird.

Wenn Sie sich nicht sicher sind, setzen Sie am Anfang jeder "nicht verwendeten" Methode ein assert (). Vielleicht wird es gerufen. Oder eine Protokollierungsanweisung.

Vielleicht ist der Code tatsächlich wertvoll. Möglicherweise handelt es sich um einen neuen Code, an dem ein Kollege seit zwei Wochen arbeitet und den er oder sie morgen einschalten würde. Es wird heute nicht angerufen, weil der Anruf morgen hinzugefügt wird.

Vielleicht ist der Code tatsächlich wertvoll Teil 2: Der Code führt möglicherweise einige sehr teure Laufzeitprüfungen durch, bei denen Fehler auftreten können. Der Code wird nur aktiviert, wenn tatsächlich etwas schief geht. Möglicherweise löschen Sie ein nützliches Debugging-Tool.

Interessanterweise ist der schlechteste Rat "Löschen. Festschreiben. Vergessen." ist am höchsten bewertet. (Codeüberprüfungen? Sie führen keine Codeüberprüfungen durch? Was in aller Welt programmieren Sie, wenn Sie keine Codeüberprüfungen durchführen?)

gnasher729
quelle
Ich bin mit Respekt anderer Meinung, dass "DELETE. COMMIT. FORGET" der schlechteste Rat ist. (Ich denke, es ist das Beste.) Ihr Ansatz ist auch in Ordnung. Ich denke, der schlechteste Rat wäre, Komponententests zu schreiben, die den "toten Code" ausüben, aber keine Behauptungen aufstellen. Das Code-Coverage-Tool wird in die Irre geführt und denkt, dass es verwendet wird.
Emory
3
Nichts in der Antwort "Löschen. Festschreiben. Vergessen" besagt, dass keine Codeüberprüfung durchgeführt werden soll. Es ist durchaus möglich (und empfehlenswert, IMHO), Code zu überprüfen, nachdem er festgeschrieben wurde (aber vor der Bereitstellung :-)).
sleske
@sleske Wie können Sie Code überprüfen, der nicht mehr vorhanden ist? In dieser Antwort wird auch nicht "Überprüfung" erwähnt.
user949300
@ user949300: Ob eine Codeänderung überprüft werden muss oder nicht, ist eine separate Frage und unabhängig davon, ob Code hinzugefügt, geändert oder gelöscht wird (das Hinzufügen von Code kann noch katastrophaler sein als das Löschen, siehe z. B. die Heartbleed-Sicherheitsanfälligkeit). Außerdem lautet die Antwort (jetzt) ​​"einen Experten hinzuziehen", was einer Codeüberprüfung ziemlich nahe kommt.
sleske
1
@ user949300: In Bezug auf "Wie überprüfen Sie Code, der nicht mehr vorhanden ist" - ich hoffe, dass dies offensichtlich ist: Betrachten Sie die Änderung in dem Tool Ihrer Wahl zur Versionskontrolle. Wie würden Sie Änderungen (etwaige Änderungen) sonst überprüfen?
sleske
6

Abhängig von der Umgebung, in der die Software ausgeführt wird, können Sie sich anmelden, wenn die Methode jemals aufgerufen wird. Wenn es nicht innerhalb eines geeigneten Zeitraums aufgerufen wird, kann die Methode sicher entfernt werden.

Dies ist ein vorsichtigerer Ansatz als nur das Löschen der Methode und kann hilfreich sein, wenn Sie in einer Umgebung mit hoher Fehleranfälligkeit arbeiten.

Wir loggen uns in einen dedizierten #unreachable-codeSlack-Channel mit einer eindeutigen Kennung für jeden zu entfernenden Kandidaten ein und es hat sich als ziemlich effektiv erwiesen.

Jamie Bull
quelle
Aber was ist, wenn es selten genannt wird (wie 1 in 1440 im Durchschnitt) ?
Peter Mortensen
dann ist "eine angemessene Zeitspanne" länger (obwohl ich einen "Mann nach Mitternacht" mag)
Jamie Bull