[CleanCode] Beispielklasse TDataLocation

[Furtbichler]

Zitat:

Delphi-Quellcode:

function TDataLocation.ConfigFile(out aConfigFileName : string; const aDefaultFileName : string = '') : Boolean;
var
   aFound : Boolean;
begin
   Result := False;
   if aDefaultFileName > '' then
     FDataFile := aDefaultFileName;
   if not Result then
     Result := FindDataCmd;
   if not Result then
     Result := FindConfigCmd;
   if not Result then
     Result := FindLocalConfig;
   if not Result then
     Result := FindLocalFile;
   if not Result then
     Result := FindUserDataFile;
   aConfigFileName := FDataFile;
end;

Würde ich anders schreiben, dann ist es kompakter, aber vielleicht nicht so leicht zu verstehen. BTW: Die Variable "aFound" ist überflüssig.

Delphi-Quellcode:

function TDataLocation.ConfigFile(out aConfigFileName : string; const aDefaultFileName : string = '') : Boolean;
begin
   if aDefaultFileName > '' then
     FDataFile := aDefaultFileName;
   try
     Result := True;
     If FindDataCmd then exit;
     If FindConfigCmd then exit;
     If FindLocalConfig then exit;
     If FindLocalFile then exit;
     If FindUserDataFile then exit;
     Result := False;
  finally
     aConfigFileName := FDataFile;
  end;
end;

[DeddyH]

Delphi-Quellcode:

{$BOOLEVAL OFF}
function TDataLocation.ConfigFile(out aConfigFileName : string; 
  const aDefaultFileName : string = ''): Boolean;
begin
  if aDefaultFileName > '' then
    FDataFile := aDefaultFileName;
  Result := FindDataCmd or FindConfigCmd or FindLocalConfig or FindLocalFile 
    or FindUserDataFile;
  aConfigFileName := FDataFile;
end;

Müsste auf dasselbe herauskommen.

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

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

[neo4a]

Zitat von Furtbichler:

Deine Lösung beschreibt den Vorgang direkt ('Wenn noch nicht erfolgreich, versuche die nächste Möglichkeit').

Ich habe aber mit einer Häufung von "if not ..."- Anweisungen gearbeitet. Empfohlen wird dagegen, immer auf positive Erfüllung hin zu prüfen, weil es sich wohl "natürlicher" und einfacher erfassen lässt.

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