Die Codebasis, an der ich arbeite, verwendet häufig Instanzvariablen, um Daten zwischen verschiedenen einfachen Methoden auszutauschen. Der ursprüngliche Entwickler ist der festen Überzeugung, dass dies den Best Practices entspricht, die im Clean Code- Buch von Onkel Bob / Robert Martin aufgeführt sind: "Die erste Regel für Funktionen lautet, dass sie klein sein sollten." und "Die ideale Anzahl von Argumenten für eine Funktion ist null (niladisch). (...) Argumente sind schwer. Sie erfordern viel konzeptionelle Kraft."
Ein Beispiel:
public class SomeBusinessProcess {
@Inject private Router router;
@Inject private ServiceClient serviceClient;
@Inject private CryptoService cryptoService;
private byte[] encodedData;
private EncryptionInfo encryptionInfo;
private EncryptedObject payloadOfResponse;
private URI destinationURI;
public EncryptedResponse process(EncryptedRequest encryptedRequest) {
checkNotNull(encryptedRequest);
getEncodedData(encryptedRequest);
getEncryptionInfo();
getDestinationURI();
passRequestToServiceClient();
return cryptoService.encryptResponse(payloadOfResponse);
}
private void getEncodedData(EncryptedRequest encryptedRequest) {
encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
}
private void getEncryptionInfo() {
encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
}
private void getDestinationURI() {
destinationURI = router.getDestination().getUri();
}
private void passRequestToServiceClient() {
payloadOfResponse = serviceClient.handle(destinationURI, encodedData, encryptionInfo);
}
}
Ich würde das in das Folgende umgestalten, indem ich lokale Variablen verwende:
public class SomeBusinessProcess {
@Inject private Router router;
@Inject private ServiceClient serviceClient;
@Inject private CryptoService cryptoService;
public EncryptedResponse process(EncryptedRequest encryptedRequest) {
checkNotNull(encryptedRequest);
byte[] encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
EncryptionInfo encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
URI destinationURI = router.getDestination().getUri();
EncryptedObject payloadOfResponse = serviceClient.handle(destinationURI, encodedData,
encryptionInfo);
return cryptoService.encryptResponse(payloadOfResponse);
}
}
Dies ist kürzer, eliminiert die implizite Datenkopplung zwischen den verschiedenen trivialen Methoden und beschränkt die Gültigkeitsbereiche der Variablen auf das erforderliche Minimum. Trotz dieser Vorteile kann ich den ursprünglichen Entwickler immer noch nicht davon überzeugen, dass dieses Refactoring gerechtfertigt ist, da es den oben erwähnten Praktiken von Onkel Bob zu widersprechen scheint.
Daher meine Fragen: Was ist das objektive wissenschaftliche Argument, um lokale Variablen vor Instanzvariablen zu bevorzugen? Ich kann es nicht genau sagen. Meine Intuition sagt mir, dass versteckte Kopplungen schlecht sind und dass ein enger Bereich besser ist als ein breiter. Aber was ist die Wissenschaft, um dies zu unterstützen?
Und umgekehrt, gibt es Nachteile bei diesem Refactoring, die ich möglicherweise übersehen habe?
quelle
Antworten:
Scope ist kein binärer Zustand, sondern ein Gradient. Sie können diese vom größten zum kleinsten ordnen:
Bearbeiten: Was ich als "Klassenbereich" bezeichne, ist das, was Sie mit "Instanzvariable" meinen. Soweit ich weiß, sind das auch, aber ich bin ein C # -Entwickler, kein Java-Entwickler. Der Kürze halber habe ich alle Statiken in die globale Kategorie zusammengefasst, da Statiken nicht das Thema der Frage sind.
Je kleiner der Umfang, desto besser. Das Grundprinzip ist, dass Variablen im kleinstmöglichen Umfang leben sollten . Das hat viele Vorteile:
Name
Eigenschaft, sind Sie nicht gezwungen , sie wie PräfixFooName
,BarName
... So Ihre Variablennamen so sauber und lapidare wie möglich zu halten.passRequestToServiceClient()
an den Anfang der Methode gelangen, und die Methode würde trotzdem kompiliert. Bei Einheimischen konnte man diesen Fehler nur machen, wenn man eine nicht initialisierte Variable übergab, was hoffentlich so offensichtlich ist, dass man es tatsächlich nicht tut.Das Problem hierbei ist, dass Ihr Argument für lokale Variablen gültig ist, Sie jedoch zusätzliche Änderungen vorgenommen haben, die nicht korrekt sind und dazu führen, dass die vorgeschlagene Korrektur den Geruchstest nicht besteht.
Obwohl ich Ihren Vorschlag "Keine Klassenvariable" verstehe und er von Vorteil ist, haben Sie auch die Methoden selbst entfernt, und das ist ein ganz anderes Ballspiel. Die Methoden sollten erhalten bleiben, und stattdessen sollten Sie sie so ändern, dass sie ihren Wert zurückgeben, anstatt ihn in einer Klassenvariablen zu speichern:
Ich stimme dem zu, was Sie in der
process
Methode getan haben , aber Sie hätten die privaten Untermethoden aufrufen sollen, anstatt ihre Körper direkt auszuführen.Sie möchten diese zusätzliche Abstraktionsebene, insbesondere wenn Sie auf Methoden stoßen, die mehrmals wiederverwendet werden müssen. Auch wenn Sie Ihre Methoden derzeit nicht wiederverwenden , ist es immer noch eine gute Praxis, bereits Untermethoden zu erstellen, wenn dies relevant ist, auch wenn dies nur die Lesbarkeit des Codes fördert.
Unabhängig vom Argument der lokalen Variablen ist mir sofort aufgefallen, dass der von Ihnen vorgeschlagene Fix erheblich schlechter lesbar ist als das Original. Ich gebe zu, dass die willkürliche Verwendung von Klassenvariablen auch die Lesbarkeit des Codes beeinträchtigt, aber nicht auf den ersten Blick, verglichen mit der Tatsache, dass Sie die gesamte Logik in einer einzigen (jetzt langwierigen) Methode gestapelt haben.
quelle
Der ursprüngliche Code verwendet Elementvariablen wie Argumente. Wenn er sagt, dass die Anzahl der Argumente minimiert werden soll, bedeutet das, dass die Datenmenge, die die Methoden benötigen, um zu funktionieren, minimiert werden soll. Das Einfügen dieser Daten in Mitgliedsvariablen verbessert nichts.
quelle
process.Start();
odermyString.ToLowerCase()
sollten nicht zu seltsam erscheinen (und sind in der Tat am einfachsten zu verstehen).this
. Man könnte sogar argumentieren, dass dieses Argument explizit angegeben wird - vor dem Punkt.Andere Antworten haben die Vorteile lokaler Variablen bereits perfekt erklärt. Alles, was bleibt, ist dieser Teil Ihrer Frage:
Das sollte einfach sein. Zeigen Sie ihm einfach das folgende Zitat in Onkel Bobs Clean Code:
Das heißt, Onkel Bob sagt nicht nur, dass eine Funktion nur wenige Argumente annehmen sollte, er sagt auch, dass Funktionen nach Möglichkeit die Interaktion mit nicht lokalen Zuständen vermeiden sollten.
quelle
"Es widerspricht der Meinung eines Onkels" ist NIE ein gutes Argument. NOCH NIE. Nimm nicht die Weisheit von Onkeln, denke selbst.
Allerdings sollten Instanzvariablen verwendet werden, um Informationen zu speichern, die tatsächlich permanent oder semipermanent gespeichert werden müssen. Die Informationen hier sind nicht. Es ist sehr einfach, ohne die Instanzvariablen zu leben, damit sie gehen können.
Test: Schreiben Sie für jede Instanzvariable einen Dokumentationskommentar. Kannst du etwas schreiben, das nicht völlig sinnlos ist? Schreiben Sie einen Dokumentationskommentar an die vier Accessoren. Sie sind ebenso sinnlos.
Das Schlimmste ist, nehmen Sie an, wie Sie Änderungen entschlüsseln, da Sie einen anderen cryptoService verwenden. Anstatt vier Codezeilen zu ändern, müssen Sie vier Instanzvariablen durch unterschiedliche, vier Getter durch unterschiedliche ersetzen und vier Codezeilen ändern.
Aber natürlich ist die erste Version vorzuziehen, wenn Sie nach der Codezeile bezahlt werden. 31 Zeilen statt 11 Zeilen. Dreimal mehr Zeilen zum Schreiben und für immer warten, zum Lesen, wenn Sie etwas debuggen, zum Anpassen, wenn Änderungen erforderlich sind, zum Duplizieren, wenn Sie einen zweiten cryptoService unterstützen.
(Verpasste den wichtigen Punkt, dass die Verwendung lokaler Variablen Sie dazu zwingt, die Aufrufe in der richtigen Reihenfolge durchzuführen).
quelle
Instanzvariablen dienen zur Darstellung der Eigenschaften ihres Host-Objekts und nicht zur Darstellung von Eigenschaften, die für Threads der Berechnung spezifischer sind als das Objekt selbst. Einige der Gründe für eine solche Unterscheidung, die offenbar noch nicht behandelt wurden, liegen in der Parallelität und Wiedereintrittsquote. Wenn Methoden Daten austauschen, indem die Werte von Instanzvariablen festgelegt werden, können zwei gleichzeitige Threads die Werte für diese Instanzvariablen leicht überfrachten, was zu zeitweise auftretenden, schwer zu findenden Fehlern führt.
Sogar ein Thread alleine kann auf Probleme in dieser Richtung stoßen, da ein hohes Risiko besteht, dass ein Datenaustauschmuster, das auf Instanzvariablen beruht, Methoden nicht mehr wiedereintrittsfähig macht. Wenn dieselben Variablen zum Übertragen von Daten zwischen verschiedenen Methodenpaaren verwendet werden, besteht das Risiko, dass ein einzelner Thread, der auch eine nicht rekursive Kette von Methodenaufrufen ausführt, auf Fehler stößt, die sich auf unerwartete Änderungen der beteiligten Instanzvariablen beziehen.
Um in einem solchen Szenario zuverlässig korrekte Ergebnisse zu erzielen, müssen Sie entweder separate Variablen für die Kommunikation zwischen den Methodenpaaren verwenden, bei denen eine die andere aufruft, oder bei jeder Methodenimplementierung alle Implementierungsdetails der anderen berücksichtigen Methoden, die direkt oder indirekt aufgerufen werden. Das ist spröde und schuppt schlecht.
quelle
public EncryptedResponse process(EncryptedRequest encryptedRequest)
ist nicht synchronisiert, und gleichzeitige Aufrufe können die Werte der Instanzvariablen beeinträchtigen. Dies ist ein guter Punkt, den Sie ansprechen sollten.Das
process(...)
Beispiel Ihrer Kollegen ist im Sinne der Geschäftslogik weitaus besser lesbar. Umgekehrt benötigt Ihr Gegenbeispiel mehr als einen flüchtigen Blick, um eine Bedeutung herauszufinden.Ungeachtet dessen ist sauberer Code sowohl lesbar als auch von guter Qualität. Wenn Sie den lokalen Status in einen globaleren Bereich verschieben, handelt es sich nur um eine Assembly auf hoher Ebene, also um eine Qualitätsnull.
Dies ist eine Wiedergabe, die die Notwendigkeit von Variablen in jedem Bereich beseitigt. Ja, der Compiler generiert sie, aber das Wichtigste ist, dass er dies steuert, damit der Code effizient ist. Während auch relativ gut lesbar.
Nur ein Punkt zur Namensgebung. Sie möchten den kürzesten Namen, der aussagekräftig ist und die bereits verfügbaren Informationen erweitert. dh destinationURI, der 'URI' ist bereits durch die Typensignatur bekannt.
quelle
Ich würde nur diese Variablen und privaten Methoden insgesamt entfernen. Hier ist mein Refactor:
Für private Methoden ist zB
router.getDestination().getUri()
übersichtlicher und lesbarer alsgetDestinationURI()
. Ich würde das sogar einfach wiederholen, wenn ich dieselbe Zeile zweimal in derselben Klasse verwende. Um esgetDestinationURI()
anders zu sehen, wenn es ein Bedürfnis gibt , dann gehört es wahrscheinlich in eine andere Klasse, nicht in dieSomeBusinessProcess
Klasse.Für Variablen und Eigenschaften müssen sie häufig Werte enthalten, die später verwendet werden sollen. Wenn die Klasse keine öffentliche Schnittstelle für die Eigenschaften hat, sollten sie wahrscheinlich keine Eigenschaften sein. Die schlechteste Art der Verwendung von Klasseneigenschaften ist wahrscheinlich die Übergabe von Werten zwischen privaten Methoden als Nebeneffekt.
Wie auch immer, die Klasse muss nur tun
process()
und dann wird das Objekt weggeworfen, es ist nicht erforderlich, einen Zustand im Speicher zu behalten. Ein weiteres Refactor-Potenzial wäre, den CryptoService aus dieser Klasse herauszunehmen.Basierend auf Kommentaren möchte ich hinzufügen, dass diese Antwort auf der Praxis der realen Welt basiert. Tatsächlich ist das erste, was ich bei der Codeüberprüfung herausgreifen würde, die Umgestaltung der Klasse und die Verlagerung der Ver- / Entschlüsselungsarbeit. Sobald dies erledigt ist, würde ich fragen, ob die Methoden und Variablen benötigt werden, ob sie korrekt benannt sind und so weiter. Der endgültige Code wird dem wahrscheinlich näher kommen:
Mit dem obigen Code glaube ich nicht, dass er weiteren Refactor benötigt. Ich denke, dass es Erfahrung braucht, um zu wissen, wann und wann man sie nicht anwendet. Regeln sind keine Theorien, die nachweislich in allen Situationen funktionieren.
Andererseits hat die Codeüberprüfung einen großen Einfluss darauf, wie lange ein Teil des Codes vergehen kann. Mein Trick ist, weniger Code zu haben und es einfach zu verstehen. Ein Variablenname kann ein Diskussionspunkt sein. Wenn ich ihn entfernen kann, müssen die Prüfer nicht einmal darüber nachdenken.
quelle
Flaters Antwort deckt Fragen des Scoping ziemlich gut ab, aber ich denke, dass es auch hier eine andere Frage gibt.
Beachten Sie, dass es einen Unterschied zwischen einer Funktion, die Daten verarbeitet, und einer Funktion gibt, die einfach auf Daten zugreift .
Ersteres führt die eigentliche Geschäftslogik aus, während letzteres die Eingabe spart und möglicherweise die Sicherheit erhöht, indem eine einfachere und wiederverwendbarere Schnittstelle hinzugefügt wird.
In diesem Fall scheinen die Datenzugriffsfunktionen die Eingabe nicht zu speichern und werden nirgendwo wiederverwendet (oder es gibt andere Probleme beim Entfernen). Diese Funktionen sollten also einfach nicht existieren.
Nur die Business - Logik in benannte Funktionen , indem sie , so erhalten wir das Beste aus beiden Welten (irgendwo zwischen Flater Antwort und Antwort des imel96 ):
quelle
Das Erste und Wichtigste: Onkel Bob scheint manchmal wie ein Prediger zu sein, gibt aber an, dass es Ausnahmen von seinen Regeln gibt.
Die ganze Idee von Clean Code ist es, die Lesbarkeit zu verbessern und Fehler zu vermeiden. Es gibt mehrere Regeln, die sich gegenseitig verletzen.
Sein Argument zu Funktionen ist, dass niladische Funktionen am besten sind, jedoch bis zu drei Parameter akzeptabel sind. Ich persönlich finde, dass 4 auch ok sind.
Wenn Instanzvariablen verwendet werden, sollten sie eine kohärente Klasse bilden. Das heißt, die Variablen sollten in vielen, wenn nicht allen nicht statischen Methoden verwendet werden.
Variablen, die an vielen Stellen der Klasse nicht verwendet werden, sollten verschoben werden.
Ich würde weder die ursprüngliche noch die überarbeitete Version für optimal halten, und @Flater hat bereits sehr gut angegeben, was mit Rückgabewerten gemacht werden kann. Es verbessert die Lesbarkeit und reduziert Fehler bei der Verwendung von Rückgabewerten.
quelle
Lokale Variablen reduzieren den Gültigkeitsbereich, begrenzen daher die Verwendungsmöglichkeiten der Variablen und tragen somit zur Vermeidung bestimmter Fehlerklassen bei und verbessern die Lesbarkeit.
Instanzvariable reduziert die Art und Weise, in der die Funktion aufgerufen werden kann, was auch dazu beiträgt, bestimmte Fehlerklassen zu reduzieren und die Lesbarkeit zu verbessern.
Zu sagen, dass das eine richtig und das andere falsch ist, kann in jedem Einzelfall eine gültige Schlussfolgerung sein, aber als allgemeiner Ratschlag ...
TL; DR: Ich denke, der Grund, warum du zu viel Eifer riechst, ist, dass es zu viel Eifer gibt.
quelle
Obwohl Methoden, die mit get ... beginnen, nicht ungültig sein sollten, wird in der ersten Lösung die Trennung der Abstraktionsebenen innerhalb der Methoden angegeben. Obwohl die zweite Lösung umfangreicher ist, ist es immer noch schwieriger zu überlegen, was in der Methode vor sich geht. Die Zuweisungen von lokalen Variablen sind hier nicht erforderlich. Ich würde die Methodennamen beibehalten und den Code so umgestalten:
quelle
Beide Dinge tun dasselbe und die Leistungsunterschiede sind nicht zu bemerken, daher gibt es meines Erachtens kein wissenschaftliches Argument. Es kommt dann auf subjektive Vorlieben an.
Und auch ich neige dazu, Ihren Weg besser zu mögen als den Ihres Kollegen. Warum? Weil ich denke, es ist einfacher zu lesen und zu verstehen, trotz der Aussagen einiger Buchautoren.
Beide Wege bewirken das Gleiche, aber sein Weg ist weiter verbreitet. Um diesen Code zu lesen, müssen Sie zwischen verschiedenen Funktionen und Elementvariablen hin und her wechseln. Es ist nicht alles an einem Ort zusammengefasst, Sie müssen sich an all das in Ihrem Kopf erinnern, um es zu verstehen. Es ist eine viel größere kognitive Belastung.
Im Gegensatz dazu packt Ihr Ansatz alles viel dichter, aber nicht so, dass es undurchdringlich wird. Sie lesen es einfach Zeile für Zeile und müssen sich nicht so viel merken, um es zu verstehen.
Wenn er es jedoch gewohnt ist , Code so zu schreiben, kann ich mir vorstellen, dass es für ihn umgekehrt sein könnte.
quelle