[CleanCode] Beispielklasse TDataLocation

[Furtbichler]

Zitat von neo4a:

Zitat von DeddyH:

Müsste auf dasselbe herauskommen.

Sehe ich auch so: Es macht aus 7 Zeilen 1, ohne dass die Lesbarkeit wirklich leidet.

Ich nicht, weil:
1. Die Auswertereihenfolge ist compilerabhängig.
2. Die Funktion ist abhängig vom Compilerschalter ('Complete boolean evaluation')

Absolute nicht 'clean code' tauglich.

[neo4a]

Zitat von Furtbichler:

Zitat von neo4a:

Zitat von DeddyH:

Müsste auf dasselbe herauskommen.

Sehe ich auch so: Es macht aus 7 Zeilen 1, ohne dass die Lesbarkeit wirklich leidet.

Ich nicht, weil:
1. Die Auswertereihenfolge ist compilerabhängig.
2. Die Funktion ist abhängig vom Compilerschalter ('Complete boolean evaluation')

Absolute nicht 'clean code' tauglich.

Ich bin hier kein "CleanCode-Schiedsrichter", aber ich stimme Dir zu und habe das Listing entsprechend angepasst. Deine Lösung lässt sich auch etwas besser Debuggen/Nachverfolgen. Einzig der exit-Sprung zur Zuweisung der out-Variablen im finally-Block behagt mir nicht so ganz.

[Furtbichler]

Hi, ich sage bereits, das die Alternative mit 'exit' kompakter, aber vielleicht nicht lesbarer ist (worum es beim CC ja auch geht).

Der Grund ist:
Deine Lösung beschreibt den Vorgang direkt ('Wenn noch nicht erfolgreich, versuche die nächste Möglichkeit').
Bei meiner Lösung muss man wissen, das ein "exit" (dagegen ist an sich nichts einzuwenden) in den finally-Abschnitt springt. Die Aussage ist 'Probiers aus und wenn es geklappt hat, fertig'.

Man könnte auch mit Exceptions arbeiten ('Probiere nächsten Schritt und wenns klappt, Abbruch').

[himitsu]

Zitat von Furtbichler:

Man könnte auch mit Exceptions arbeiten ('Probiere nächsten Schritt und wenns klappt, Abbruch').

Ein Exit innerhalb eines Try-Finally löst eine stille Exception aus, um in das Finally zu springen.

Aber so im Allgemeinen (abgesehn von diese impliziten Exit-Exception und von EAbort) sind Exceptions/Ausnahmen nicht für eine explizite Programmsteuerung vorgesehn. Und ich bin mir fast sicher, daß selbst CleanCode was dagegen hätte.


Wer FullBooleanEval global aktiviert, ist selber Schuld und muß mit den Konsequenzen leben.
Wenn das wirklich mal benötigt würde, dann sollte man es nur lokal aktivieren.

Aber wer als Entwickler sich absichern will, kann sowas ja nochmal explizit angeben, am Anfang seiner Dateien, bzw. in einer globalen Include-Datei.

[Uwe Raabe]

Zitat von Furtbichler:

1. Die Auswertereihenfolge ist compilerabhängig.
2. Die Funktion ist abhängig vom Compilerschalter ('Complete boolean evaluation')

Absolute nicht 'clean code' tauglich.

Ich kann das Clean Code Buch jetzt nicht auswendig, aber meines Wissens steht da weder was über "Clean Code ist portabel", noch "Clean Code ist resistent gegen Umgebungsänderungen". Im Zweifelsfall prüft man das mit {$IFOPT B+} und wirft einen Compilerfehler.

Was die Auswertereihenfolge betrifft: Ein Compiler, der ein Konstrukt wie if (P<> nil) and P.Enabled then in eine Schutzverletzung führt, hat bei m.E. sowieso keine Chancen.

[himitsu]

if Assinged(P) then if P.Enabled then sieht och nicht besser aus, aber wenn Clean-Code sowas verlangt, dann bin ich für Dirty-Code.

[Furtbichler]

Eine Diskussion über den Einsatz von 'exit', und ob ein 'exit' eine Ausnahme darstellt (oder nur intern so compiliert wird), hatten wir schon oft und driftet in Richtung 'Religion' ab.

Eine 'Ausnahme' im Programmfluss ist ja nichts anderes als das Gegenteil der Regel. Es ist sehr wohl "sauberer Code", mit Ausnahmen zu arbeiten.
Der Grund ist recht einfach: Durch die Verwendung von Exceptions kann ich den normalen Programmfluss viel direkter beschreiben.

Hier könnte man durchaus -ähnlich dem 'exit' Konstrukt- mit Exceptions (na, vielleicht EAbort) arbeiten. Ich würde die aufgerufenen 'FindXXX'-Methoden umbenennen, damit klar wird, das etwas versucht wird. Die Methoden liefern dann kein Bool, sondern werfen eine EAbort-Exception.

Delphi-Quellcode:

function TDataLocation.ConfigFile(out aConfigFileName : string; const aDefaultFileName : string = '') : Boolean;
begin
    if aDefaultFileName > '' then
      FDataFile := aDefaultFileName;
    try
      Result := True;
      TryToFindDataCmd;
      TryToFindConfigCmd;
      TryToFindLocalConfig;
      TryToFindLocalFile;
      TryToFindUserDataFile;
      Result := False;
   finally
      aConfigFileName := FDataFile;
   end;
end;

Was mir dann aber nicht gefällt, ist die unterschiedliche Verwendung der Behandlung eines Methodenresultats. Es existieren ja (mindestens) zwei Ansätze:
* Implementieren als Funktion und zurückliefern eines Resultats
* Implementieren als Methode und werfen einer Exception

Clean Code favorisiert hier eindeutig die Verwendung von Exceptions.

Nun, wenn man in seiner Klasse / Framework schon auf Funktionsresultate à la 'hat geklappt / hat nicht geklappt' setzt, sollte man das auch so durchziehen.

Das Programmieren mit dem Ziel des 'Clean Code' ist ein ständiges Abwiegen von Varianten. Es muss sich 'gut anfühlen', und das ist in erster Linie ein subjektives Gefühl. Das heißt natürlich nicht, das CC nur aus dem guten Gefühl darstellt, es müssen auch Grundregeln eingehalten werden.

Merke: 'Clean Code' ist nur eine Bewegung, die sich Regeln auferlegt, um immer besseren (im Sinne der Regeln) Code zu produzieren.

Zitat von Uwe Raabe:

..aber meines Wissens steht da weder was über "Clean Code ist portabel", noch "Clean Code ist resistent gegen Umgebungsänderungen".

Nein, aber da steht auch 'der Code muss lesbar und selbsterklärend sein'. Das mit dem 'portabel' versteht sich von selbst. Denn 'portabel' ist gleichbedeutend mit 'robust' und 'wartbar'.

Zitat von Uwe Raabe:

Was die Auswertereihenfolge betrifft: Ein Compiler, der ein Konstrukt wie if (P<> nil) and P.Enabled then in eine Schutzverletzung führt, hat bei m.E. sowieso keine Chancen.

Stimmt, aber das steht da ja nicht. Es handelt sich um funktional unabhängigen Code ('FindXXX'), die ein gewitzter Compiler durchaus parallel oder anhand von Heuristiken in anderer Reihenfolge abarbeiten könnte.
Des Weiteren vergisst Du leider vollkommen, das die Auswertereihenfolge nur bei unabhängigen Termen machbar ist und jeder, aber auch wirklich jeder, der ein Semester Compilerbau studiert hat, bei so eine Zwischenbemerkung schallendes Gelächter geerntet hätte.

Zitat von himitsu:

Wer FullBooleanEval global aktiviert, ist selber Schuld und muß mit den Konsequenzen leben.

Jupp. Aber wenn es aus Versehen passiert ist, sollte es keine Auswirkungen auf die Funktionalität haben, gell?

Zitat:

Aber wer als Entwickler sich absichern will, kann sowas ja nochmal explizit angeben, am Anfang seiner Dateien, bzw. in einer globalen Include-Datei.

Du hast den Clean-Code Gedanken nicht verstanden.

Zitat von himitsu:

if Assinged(P) then if P.Enabled then sieht och nicht besser aus, aber wenn Clean-Code sowas verlangt, dann bin ich für Dirty-Code.

Das merkt man bei deiner Art, zu programmieren Aber nebenbei: CC verlangt keine Hosenträger, wo die Hose eng sitzt und ein Gürtel verwendet wird. Wenn Du deinen Code so schreibst, das Eingabeparameter abgefragt werden, kannst Du darauf verzichten.

Ich sags nochmal: CC hat nichts mit kompakt zu tun oder mit performant. Sondern nur mit lesbar, robust, testbar, wartbar.

[Furtbichler]

Zurück zum Thema:
Die Funktion "GetCmdLineSwitchValue" verstößt mindestens gegen die CC-Prinzipien "Command Query Separation" sowie "Do one thing": Die Funktion ermittelt den Wert eines Kommandozeilenparameters UND liefert TRUE bei Erfolg.

Ich würde es genauso umsetzen, aber 'sauber' im Sinne von CC wäre das nicht. Es ist aus dem Funktionsnamen nicht ersichtlich, das "aFilename" geliefert wird; man muss 2x hinschauen. Da es keine zeitliche Koppelung zwischen 'Vorhandensein des Parameters' und 'Extrahieren des Parameters' gibt, sollte man die Funktion aufteilen (nur im Sinne von CC)

Delphi-Quellcode:

...
   if GetCmdLineSwitchValue(aFileName, FConfigCmdParam) then
...

CC liefert hier ein Beispiel, wie es besser geht:

Delphi-Quellcode:

If CmdLineSwitchExists(FConfigCmdParam) then
  aFilename := GetCmdLineSwichValue(FConfigCmdParam);

[himitsu]

Zitat von Furtbichler:

Jupp. Aber wenn es aus Versehen passiert ist, sollte es keine Auswirkungen auf die Funktionalität haben, gell?

Grob übersetzt würde CleanCode dann aber bedeuten auf nahezu alle Codeoptimierungen zu verzichten.

Ein gewisses Standard-Verhalten würde ich einfach voraussetzen
und wenn dieses nicht zutrifft, dann darf auch keiner ein standardmäßig korrektes Verhalten voraussetzen.

Denn mit voller booleanischen Auswertung kann der Code nur in if Assinged(DS) then if DS.Active then if Assinged(DS.FindField('x')) then if not DS.FieldByName('x').IsNull then if DS.FieldByName('x').AsString = 'test' then enden.
Selbst mit Zeilenumbrüchen und Einrückung wird es nicht lesbarer.

[neo4a]

Zitat von Furtbichler:

Die Funktion "GetCmdLineSwitchValue" verstößt mindestens gegen die CC-Prinzipien "Command Query Separation" sowie "Do one thing": Die Funktion ermittelt den Wert eines Kommandozeilenparameters UND liefert TRUE bei Erfolg.

Ich würde es genauso umsetzen, aber 'sauber' im Sinne von CC wäre das nicht. Es ist aus dem Funktionsnamen nicht ersichtlich, das "aFilename" geliefert wird; man muss 2x hinschauen.

Dieser Einwand trifft ja auch zu auf die Haupt-Funktion:
function ConfigFile(out aConfigFileName : string; const aDefaultFileName : string = '') : Boolean; Aber wie Du schon vorher gesagt hast, ist CC keine Religion, sondern eher "professioneller Pragmatismus". Nun ist die Ermittlung von Kommandozeilen-Parameter kein besonders "teurer" Vorgang, so dass sie sicherlich auch 2x durchlaufen werden könnte. Ich würde es trotzdem nicht machen und lieber an der aufrufenden Stelle einen Kommentar hinterlassen.