[CleanCode] Beispielklasse TDataLocation

[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);

[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.

[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.

[Furtbichler]

Zitat von himitsu:

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

Ja. Zunächst schon. Heutzutage sind die Rechner schnell genug, und mal ehrlich: An wie vielen Stellen in einer komplexen Applikation kommt es auf Performance an? Wo es sinnvoll und nötig ist, wirft man das CC-Konzept natürlich vereinzelt über Bord. Aber mit Ansage, d.h. Kommentar.

Wichtig bei Performance ist eh das Verfahren und nicht die Codetricks.

Zitat:

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.

Wie wäre es mit einer ordentlichen Exceptionbehandlung? Ein integraler Bestandteil von CC.

Zitat von himitsu:

Größtes Gegenargument gegen Exceptions zur regulären Programmsteuerung.

Delphi-Quellcode:

try
  X := StrToInt(S);
except
  X := 0;
end;

Ein ungücklich gewähltes Gegenbeispiel, das zudem keines ist.
Was bedeutet 'Exception'? Und wieso passt das nicht zur 'regulären' Programmsteuerung? Genau, weil es Ausnahmen sind. Wenn Du in der Regel in deinem Code davon ausgehen musst, das 'S' auch keine Zahl beinhalten kann, dann solltest Du das auch so ausdrücken:

Delphi-Quellcode:

If IsANumber(S) Then
  x:=StrToInt(S);

Oder eben, nicht ganz CC, aber dafür praktischer:
if tryStrToInt(S,X) then .

Noch etwas zu Exceptions: Ich würde sie nicht im Sinne von globalen 'Goto' verwenden. Dafür können Sie nämlich verwendet werden (und eigentlich sind sie das auch). Eine gute Exception-Verwaltung versteckt die konkreten Probleme einer Implementierung (z.B. TCP-Timeout, DB-Probleme etc.) und verarbeitet sie. Falls die Exception dazu führt, den Programmfluss abbrechen zu müssen (Datenübertragung bei TCP-Timeout ist irgendwie unnütz), wird die Exception 'abstrahiert' nach oben gereicht. Der aufrufenden Schicht ist es egal, warum die Übertragung nicht funktioniert hat (Details stehen im Log), nur DAS sie nicht funktioniert hat, ist entscheidend. Übrigens kann eine Datenübertragung auch per RS-232, Profibus o.ä. ablaufen. Insofern ist es problematisch, den technischen Grund (oder: die ursprüngliche Exception) weiterzureichen: Bei einer nachträglichen Erweiterung ist nicht gesichert, das der Code noch funktioniert, denn es könnte sein, das nur spezielle Exceptions abgefangen werden (ETCPTimeoutException z.B.)

[himitsu]

Das "Codeoptimierungen" bezog sich nicht grundsätzlich auf die Performance.

Selbst das einfache if Assigned(V) and V.Xyz then gilt doch auch schon als Sowas.

PS: Von der Funktion her ist hier natürlich StrToIntDef die direkte Übersetzung.
IsANumber und TryStrToInt, zusammen mit einem IF, wäre mehr die umständlichere logische Übersetzung.

[Furtbichler]

himitsu, wir reden nicht von Code, den man in der Praxis verwenden würde, sondern von Code, der dem 'Clean-Code' Prinzip entspricht. Und das verteufelt nun mal Funktionen wie 'StrToIntDef' oder 'TryStrToInt', alleine schon wegen dem Namen, aber vor allen Dingen der oben bereits ausführlich aufgeführten Gründen wegen.

Dir liegt Clean-Code nicht, das heißt doch aber nicht, das Du dann keinen guten Code zu Papier bringen kannst.

Lies mal das Buch, ich vermute nämlich, das Du es nicht kennst. Es ist sehr empfehlenswert.

In die gleiche Kerbe schlägt übrigens Quasar.

[Stevie]

Ich bin der Meinung, dass solche TryWasAuchImmer(out Wert) und WasAuchImmerDef(DefaultWert) Routinen ihre Daseinsberechtigung haben und keineswegs im Widerspruch zu Clean Code stehen.
Was wäre die Alternative? Am Beispiel TryStrToInt verdeutlicht müsste man entweder testen, ob der gegebene Stringwert in einen Integer gecastet werden kann oder die Exception abhandeln - überall, wo man einen String in einen Integer umwandeln muss - bisschen DRY verletzt oder?

Anderes Einsatzgebiet der Try... Methoden ist bei mir immer, wenn es sein kann, dass ich ein Objekt zurückbekomme, oder auch nicht (z.b. bei der Reflection ein TryGetProperty(PropertyName)). Somit fehlt ein zusätzliches Überprüfen auf Assigned.

Aber auch hierzu gibt es unterschiedliche Meinungen.

Zum Thema Exceptions - zu deutsch Ausnahmen - sie heißen nicht aus Zufall so. Sie sind nicht dazu da, den flow control des Programms zu steuern!

[Furtbichler]

Zitat von Stevie:

Ich bin der Meinung, dass solche ... Routinen ihre Daseinsberechtigung haben und keineswegs im Widerspruch zu Clean Code stehen.

Vom Wesen sehe ich das auch so, aber Dr.Bob zeigt genau so ein Beispiel in seiner 'Bibel'.

Zitat:

Zum Thema Exceptions - zu deutsch Ausnahmen - sie heißen nicht aus Zufall so. Sie sind nicht dazu da, den flow control des Programms zu steuern!

Amen, vollkommen richtig. Allerdings verwendest Du sie zum 'steuern' des Programmflusses, da dieser unterbrochen wird um kontrolliert an einer definierten Stelle, nämlich dem Except-Block, weiterzulaufen. So gesehen ist das 'steuern'.

Das ist vielleicht Wortklauberei, aber das darf an dieser Stelle (und auf diesem Niveau) ruhig erlaubt sein.