Gründe, um lokale Variablen vor Instanzvariablen zu bevorzugen?

109

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?

Alexander
quelle
Kommentare sind nicht für eine längere Diskussion gedacht. Diese Unterhaltung wurde in den Chat verschoben .
maple_shaft

Antworten:

170

Was ist das objektive wissenschaftliche Argument, um lokale Variablen vor Instanzvariablen zu bevorzugen?

Scope ist kein binärer Zustand, sondern ein Gradient. Sie können diese vom größten zum kleinsten ordnen:

Global > Class > Local (method) > Local (code block, e.g. if, for, ...)

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:

  • Es zwingt Sie, über die Verantwortung der aktuellen Klasse nachzudenken, und hilft Ihnen, sich an die SRP zu halten.
  • Es ermöglicht Ihnen, nicht globale Namenskonflikte vermeiden müssen, zB wenn zwei oder mehr Klassen eine haben NameEigenschaft, sind Sie nicht gezwungen , sie wie Präfix FooName, BarName... So Ihre Variablennamen so sauber und lapidare wie möglich zu halten.
  • Der Code wird deklariert, indem die verfügbaren Variablen (z. B. für Intellisense) auf diejenigen beschränkt werden, die für den Kontext relevant sind.
  • Es ermöglicht eine Form der Zugriffskontrolle, sodass Ihre Daten nicht von einem unbekannten Akteur (z. B. einer anderen von einem Kollegen entwickelten Klasse) manipuliert werden können.
  • Dadurch wird der Code besser lesbar, da Sie sicherstellen, dass die Deklaration dieser Variablen so nah wie möglich an der tatsächlichen Verwendung dieser Variablen bleibt.
  • Das willkürliche Deklarieren von Variablen in einem zu weiten Bereich weist häufig auf einen Entwickler hin, der OOP nicht genau versteht oder nicht weiß, wie man es implementiert. Das Anzeigen von Variablen mit einem zu großen Gültigkeitsbereich ist eine rote Fahne dafür, dass mit dem OOP-Ansatz möglicherweise etwas nicht stimmt (entweder mit dem Entwickler im Allgemeinen oder mit der Codebasis im Besonderen).
  • (Kommentar von Kevin) Die Verwendung von Einheimischen zwingt Sie dazu, die Dinge in der richtigen Reihenfolge zu tun. Im ursprünglichen (Klassenvariablen-) Code könnten Sie fälschlicherweise 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.

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.

Und umgekehrt, gibt es Nachteile bei diesem Refactoring, die ich möglicherweise übersehen habe?

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:

private byte[] getEncodedData() {
    return cryptoService.decryptRequest(encryptedRequest, byte[].class);
}

private EncryptionInfo getEncryptionInfo() {
    return cryptoService.getEncryptionInfoForDefaultClient();
}

// and so on...

Ich stimme dem zu, was Sie in der processMethode getan haben , aber Sie hätten die privaten Untermethoden aufrufen sollen, anstatt ihre Körper direkt auszuführen.

public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    byte[] encodedData = getEncodedData();
    EncryptionInfo encryptionInfo = getEncryptionInfo();

    //and so on...

    return cryptoService.encryptResponse(payloadOfResponse);
}

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.

Flater
quelle
Kommentare sind nicht für eine längere Diskussion gedacht. Diese Unterhaltung wurde in den Chat verschoben .
maple_shaft
79

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.

Alex
quelle
20
Stimme voll und ganz zu! Diese Mitgliedsvariablen sind einfach implizite Funktionsargumente. In der Tat ist es schlimmer, da es jetzt keine explizite Verbindung zwischen dem Wert dieser Variablen und der Funktionsnutzung (von einem externen POV) gibt
Rémi
1
Ich würde sagen, das ist nicht das, was das Buch bedeutet. Wie viele Funktionen benötigen genau null Eingabedaten, um ausgeführt zu werden? Dies ist einer der Punkte, von denen ich denke, dass das Buch falsch war.
Qwertie
1
@Qwertie Wenn Sie ein Objekt haben, sind die Daten, mit denen es arbeitet, möglicherweise vollständig darin eingekapselt. Funktionen wie process.Start();oder myString.ToLowerCase()sollten nicht zu seltsam erscheinen (und sind in der Tat am einfachsten zu verstehen).
R. Schmitz
5
Beide haben ein Argument: das Implizite this. Man könnte sogar argumentieren, dass dieses Argument explizit angegeben wird - vor dem Punkt.
Blackjack
47

Andere Antworten haben die Vorteile lokaler Variablen bereits perfekt erklärt. Alles, was bleibt, ist dieser Teil Ihrer Frage:

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.

Das sollte einfach sein. Zeigen Sie ihm einfach das folgende Zitat in Onkel Bobs Clean Code:

Keine Nebenwirkungen haben

Nebenwirkungen sind Lügen. Ihre Funktion verspricht, eine Sache zu tun, aber sie tut auch andere versteckte Sachen. Manchmal werden unerwartete Änderungen an den Variablen der eigenen Klasse vorgenommen. Manchmal werden sie zu den in die Funktion übergebenen Parametern oder zu Systemglobalen. In beiden Fällen handelt es sich um falsche und schädliche Irrtümer, die häufig zu merkwürdigen zeitlichen Kopplungen und Ordnungsabhängigkeiten führen.

(Beispiel weggelassen)

Dieser Nebeneffekt erzeugt eine zeitliche Kopplung. Das heißt, checkPassword kann nur zu bestimmten Zeiten aufgerufen werden (mit anderen Worten, wenn das Initialisieren der Sitzung sicher ist). Wenn es nicht in der richtigen Reihenfolge aufgerufen wird, können Sitzungsdaten versehentlich verloren gehen. Zeitliche Kopplungen sind verwirrend, besonders wenn sie als Nebeneffekt verborgen sind. Wenn Sie eine zeitliche Kopplung benötigen, sollten Sie dies im Namen der Funktion verdeutlichen. In diesem Fall könnten wir die Funktion checkPasswordAndInitializeSession umbenennen, obwohl dies mit Sicherheit gegen "Mach eins" verstößt.

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.

Meriton
quelle
3
In einer "Präfektenwelt" wäre dies die zweite aufgeführte Antwort. Die erste Antwort für die ideale Situation, in der der Mitarbeiter auf die Vernunft hört - aber wenn der Mitarbeiter ein Eiferer ist, würde diese Antwort hier die Situation ohne allzu große Unschärfe behandeln.
R. Schmitz
2
Für eine pragmatischere Variante dieser Idee ist es einfach viel einfacher, über den lokalen Status als über den instanziellen oder globalen Status zu argumentieren. Gut definierte und in sich geschlossene Mutabilität und Nebenwirkungen führen selten zu Problemen. Zum Beispiel arbeiten viele Sortierfunktionen direkt über Nebeneffekte, aber dies kann in einem lokalen Bereich leicht begründet werden.
Beefster
1
Ah, der gute alte "in einer widersprüchlichen Axiomatik ist es möglich, alles zu beweisen". Da es keine harten Wahrheiten und Lügen gibt, müssen alle Dogmen Aussagen enthalten, die das Gegenteil aussagen, dh widersprüchlich sind.
ivan_pozdeev
26

"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).

gnasher729
quelle
16
Selber denken ist natürlich gut. Ihr einleitender Absatz schließt jedoch effektiv Lernende oder Junioren ein, die den Beitrag von Lehrern oder Senioren ablehnen. das geht zu weit.
Flater
9
@Flater Nachdem Sie über die Eingabe von Lehrern oder Senioren nachgedacht haben und gesehen haben, dass sie falsch sind, ist es das einzig Richtige, diese Eingabe abzulehnen. Am Ende geht es nicht darum, etwas zu entlassen, sondern es in Frage zu stellen und es nur dann zu entlassen, wenn es sich definitiv als falsch herausstellt.
glglgl
10
@ChristianHackl: Ich bin voll dabei und folge nicht blindlings dem Traditionalismus, aber ich würde ihn auch nicht blindlings abweisen. Die Antwort scheint zu empfehlen, erworbenes Wissen zugunsten Ihrer eigenen Sichtweise zu meiden, und das ist kein gesunder Ansatz. Blind anderen Meinungen folgen, nein. Fragen Sie etwas, wenn Sie nicht einverstanden sind, natürlich ja. Ich lehne es ab, weil Sie nicht einverstanden sind, nein. Wie ich es gelesen habe, scheint der erste Absatz der Antwort zumindest Letzteres nahezulegen. Es kommt sehr darauf an, was Gnasher mit "für sich selbst denken" meint, was ausgearbeitet werden muss.
Flater
9
Auf einer Plattform zum Teilen von Weisheiten scheint dies ein wenig fehl am Platz zu sein ...
drjpizzle
4
NIE ist auch NIE ein gutes Argument ... Ich stimme voll und ganz zu, dass Regeln existieren, um zu führen, nicht um zu verschreiben. Regeln über Regeln sind jedoch nur eine Unterklasse von Regeln. Sie haben also Ihr eigenes Urteil gesprochen ...
6.
14

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?

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.

John Bollinger
quelle
7
Bisher ist dies die einzige Antwort, die Thread-Sicherheit und Parallelität erwähnt. Das ist angesichts des speziellen Codebeispiels in der Frage erstaunlich: Eine Instanz von SomeBusinessProcess kann nicht sicher mehrere verschlüsselte Anforderungen gleichzeitig verarbeiten. Die Methode 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.
Joshua Taylor
9

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.

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest request) {
    checkNotNull(encryptedRequest);

    return encryptResponse
      (routeTo
         ( destination()
         , requestData(request)
         , destinationEncryption()
         )
      );
  }

  private byte[] requestData(EncryptedRequest encryptedRequest) {
    return cryptoService.decryptRequest(encryptedRequest, byte[].class);
  }

  private EncryptionInfo destinationEncryption() {
    return cryptoService.getEncryptionInfoForDefaultClient();
  }

  private URI destination() {
    return router.getDestination().getUri();
  }

  private EncryptedObject routeTo(URI destinationURI, byte[] encodedData, EncryptionInfo encryptionInfo) {
    return serviceClient.handle(destinationURI, encodedData, encryptionInfo);
  }

  private void encryptResponse(EncryptedObject payloadOfResponse) {
    return cryptoService.encryptResponse(payloadOfResponse);
  }
}

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.

Kain0_0
quelle
4
Durch das Eliminieren aller Variablen wird der Code nicht unbedingt leichter lesbar.
Pharap,
Beseitigen Sie alle Variablen vollständig mit pointfree Stil en.wikipedia.org/wiki/Tacit_programming
Marcin
@Pharap Stimmt, ein Mangel an Variablen gewährleistet nicht die Lesbarkeit. In einigen Fällen wird das Debuggen sogar schwieriger. Der Punkt ist, dass gut gewählte Namen, eine klare Ausdrucksweise, eine Idee sehr klar kommunizieren können, während sie dennoch effizient sind.
Kain0_0
7

Ich würde nur diese Variablen und privaten Methoden insgesamt entfernen. Hier ist mein Refactor:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    return cryptoService.encryptResponse(
        serviceClient.handle(router.getDestination().getUri(),
        cryptoService.decryptRequest(encryptedRequest, byte[].class),
        cryptoService.getEncryptionInfoForDefaultClient()));
  }
}

Für private Methoden ist zB router.getDestination().getUri()übersichtlicher und lesbarer als getDestinationURI(). Ich würde das sogar einfach wiederholen, wenn ich dieselbe Zeile zweimal in derselben Klasse verwende. Um es getDestinationURI()anders zu sehen, wenn es ein Bedürfnis gibt , dann gehört es wahrscheinlich in eine andere Klasse, nicht in die SomeBusinessProcessKlasse.

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:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;

  public Response process(Request request) {
    return serviceClient.handle(router.getDestination().getUri());
  }
}

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.

imel96
quelle
Meine Zustimmung, obwohl viele hier zögern werden. Natürlich machen einige Abstraktionen Sinn. (BTW eine Methode namens "Prozess" ist schrecklich.) Aber hier ist die Logik minimal. Die Frage des OP betrifft jedoch einen ganzen Codestil, und dort kann der Fall komplexer sein.
Joop Eggen
1
Ein klares Problem bei der Verkettung in einen Methodenaufruf ist die Lesbarkeit. Es funktioniert auch nicht, wenn Sie mehr als eine Operation für ein bestimmtes Objekt benötigen. Außerdem ist dies nahezu unmöglich zu debuggen, da Sie nicht durch die Vorgänge gehen und die Objekte untersuchen können. Während dies auf technischer Ebene funktioniert, würde ich dies nicht befürworten, da es Nicht-Laufzeit-Aspekte der Softwareentwicklung massiv vernachlässigt.
Flater
@Flater Ich stimme Ihren Kommentaren zu, wir möchten dies nicht überall anwenden. Ich habe meine Antwort überarbeitet, um meine praktische Position zu verdeutlichen. Ich möchte zeigen, dass wir in der Praxis Regeln nur anwenden, wenn dies angemessen ist. In diesem Fall ist der Aufruf einer Verkettungsmethode in Ordnung, und wenn ich ein Debugging durchführen muss, würde ich die Tests für die verketteten Methoden aufrufen.
imel96
@JoopEggen Ja, Abstraktionen sind sinnvoll. In dem Beispiel geben die privaten Methoden sowieso keine Abstraktionen an, die Benutzer der Klasse wissen nicht einmal davon
imel96
1
@ imel96 Es ist lustig, dass Sie wahrscheinlich einer der wenigen Menschen hier sind, denen aufgefallen ist, dass die Kopplung zwischen ServiceClient und CryptoService es lohnt, sich auf das Injizieren von CS in SC anstatt in SBP zu konzentrieren und das zugrunde liegende Problem auf einer höheren Architekturebene zu lösen. Das ist IMVHO der Hauptpunkt dieser Geschichte; Es ist zu einfach, den Überblick zu behalten, während man sich auf die Details konzentriert.
Vaxquis
4

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 ):

public EncryptedResponse process(EncryptedRequest encryptedRequest) {

    byte[] requestData = decryptRequest(encryptedRequest);
    EncryptedObject responseData = handleRequest(router.getDestination().getUri(), requestData, cryptoService.getEncryptionInfoForDefaultClient());
    EncryptedResponse response = encryptResponse(responseData);

    return response;
}

// define: decryptRequest(), handleRequest(), encryptResponse()
user673679
quelle
3

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.

kap
quelle
1

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.

Drjpizzle
quelle
0

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:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    return getEncryptedResponse(
            passRequestToServiceClient(getDestinationURI(), getEncodedData(encryptedRequest) getEncryptionInfo())
        );
  }

  private EncryptedResponse getEncryptedResponse(EncryptedObject encryptedObject) {
    return cryptoService.encryptResponse(encryptedObject);
  }

  private byte[] getEncodedData(EncryptedRequest encryptedRequest) {
    return cryptoService.decryptRequest(encryptedRequest, byte[].class);
  }

  private EncryptionInfo getEncryptionInfo() {
    return cryptoService.getEncryptionInfoForDefaultClient();
  }

  private URI getDestinationURI() {
    return router.getDestination().getUri();
  }

  private EncryptedObject passRequestToServiceClient(URI destinationURI, byte[] encodedData, EncryptionInfo encryptionInfo) {
    return serviceClient.handle(destinationURI, encodedData, encryptionInfo);
  }
}
Roman Weis
quelle
0

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.

Vilx-
quelle
Das hat sich in der Tat als große Hürde für mich herausgestellt, den Fluss zu verstehen. Dieses Beispiel ist recht klein, aber ein anderes verwendete 35 (!) Instanzvariablen für die Zwischenergebnisse, ohne die Dutzend Instanzvariablen mit den Abhängigkeiten zu zählen. Es war ziemlich schwierig, dem zu folgen, da man den Überblick behalten muss, was bereits früher eingestellt wurde. Einige wurden später sogar wiederverwendet, was es noch schwieriger machte. Zum Glück haben die hier vorgebrachten Argumente meinen Kollegen endlich überzeugt, dem Refactoring zuzustimmen.
Alexander