[CleanCode] Beispielklasse TDataLocation

[neo4a]

Eine frühere Diskussion über Delphi-Spring führte über CleanCode-Design zu Marx-Zitaten und dem Wunsch zu mehr Sachdienlichkeit. Ich stelle hier eine 200-Zeilen-Klasse zur Diskussion, die ein wohl häufig auftretendes Problem angeht: Wo sucht und speichert eine App ihre Daten?

Einmal abgesehen vom Nutzwert (portable App, CommandLine-Parameter, ConfigFile) geht es mir darum, praktisches Delphi-CleanCode-Design zu diskutieren und bei Interesse weiterführend noch die Einbindung mittels Spring-DI zu zeigen.

Die Klasse TDataLocation selbst veröffentlicht nichts als den Konstruktor. Alle Schnittstellen werden über die Interface-Deklaration (hier Fluent-Interface anstatt Properties mit Getter/Setter) veröffentlicht.

Im Ausgangs-Design muss die Unit uDataLocation zwar noch dort eingebunden werden, wo die Funktionalität benötigt wird. Mittels Spring-DI wird mit geringer Anpassung dann eine völlige Entkopplung möglich sein. Insgesamt fehlt zwar noch etwas Funktionalität und die Klasse bindet auch noch eine Hilfs-Unit (uUtils für GetCmdLineSwitchValue() und GetSpecialFolder()) ein, das habe ich aber insbesondere in Kauf genommen, um das 200-Zeilen-Limit einhalten zu können.

Delphi-Quellcode:

unit uDataLocation;

interface

type
  IDataLocation = interface
    ['{72C10C78-A39E-4005-8D53-57FD44A9B7F4}']
    function ConfigFile(out aConfigFileName : string; const aDefaultFileName : string = '') : Boolean;
    function DataFileExt(aDataFileExt : string) : IDataLocation;
    function DataCmdParam(aDataCmdParam : string) : IDataLocation;
    function ConfigFileExt(aConfigFileExt : string) : IDataLocation;
    function ConfigCmdParam(aConfigCmdParam : string) : IDataLocation;
    function ConfigSection(aConfigSection : string) : IDataLocation;
    function DataFileKey(aDataFileKey : string) : IDataLocation;
  end;

type
  TDataLocation = class(TInterfacedObject, IDataLocation)
  private
    FAppName        : string;
    FDataFile       : string;
    FConfigFile     : string;
    FDataFileExt    : string;
    FDataCmdParam   : string;
    FConfigFileExt  : string;
    FConfigCmdParam : string;
    FConfigSection  : string;
    FDataKey        : string;
    FIsLocal        : Boolean;
    FIsUSB          : Boolean;

    function FindDataCmd : Boolean;
    function FindConfigCmd : Boolean;
    function FindLocalConfig : Boolean;
    function FindLocalFile : Boolean;
    function FindUserDataFile : Boolean;
  private // IdcDataLocation
    function ConfigFile(out aConfigFileName : string; const aDefaultFileName : string = '') : Boolean;
    function DataFileExt(aDataFileExt : string) : IDataLocation;
    function DataCmdParam(aDataCmdParam : string) : IDataLocation;
    function ConfigFileExt(aConfigFileExt : string) : IDataLocation;
    function ConfigCmdParam(aConfigCmdParam : string) : IDataLocation;
    function ConfigSection(aConfigSection : string) : IDataLocation;
    function DataFileKey(aDataFileKey : string) : IDataLocation;
  public
    constructor Create;
  end;

implementation

uses
  SysUtils, Forms, IniFiles,  ShlObj,
  uUtils;

const
  cDataFileExt   = '.zip';
  cDataCmdParam  = 'data';
  cConfigFileExt = '.ini';
  cConfigCmd     = 'config';
  cConfigSection = 'Config';
  cDataFileKey   = 'Data';

function TDataLocation.ConfigCmdParam(aConfigCmdParam: string): IDataLocation;
begin
  Result          := Self;
  FConfigCmdParam := aConfigCmdParam;
end;

function TDataLocation.ConfigFile(out aConfigFileName : string; const aDefaultFileName : string = '') : Boolean;
begin
   Result := True;
   if aDefaultFileName > '' then
     FDataFile := aDefaultFileName;
   try
     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;

function TDataLocation.ConfigFileExt(aConfigFileExt: string): IDataLocation;
begin
  Result         := Self;
  FConfigFileExt := aConfigFileExt;
  FConfigFileExt := ChangeFileExt(FAppName, FConfigFileExt);
end;

function TDataLocation.ConfigSection(aConfigSection: string): IDataLocation;
begin
  Result         := Self;
  FConfigSection := aConfigSection;
end;

constructor TDataLocation.Create;
begin
  FAppName        := Application.ExeName;
  FDataFileExt    := cDataFileExt;
  FDataCmdParam   := cDataCmdParam;
  FConfigFileExt  := cConfigFileExt;
  FConfigCmdParam := cConfigCmd;
  FConfigSection  := cConfigSection;
  FDataKey        := cDataFileKey;
  FIsUSB          := False;
  FIsLocal        := True;
  FDataFile       := ChangeFileExt(FAppName, FDataFileExt);
  FConfigFile     := ChangeFileExt(FAppName, FConfigFileExt);
end;

function TDataLocation.DataCmdParam(aDataCmdParam: string): IDataLocation;
begin
  Result        := Self;
  FDataCmdParam := aDataCmdParam;
end;

function TDataLocation.DataFileExt(aDataFileExt : string): IDataLocation;
begin
  Result       := Self;
  FDataFileExt := aDataFileExt;
  FDataFile    := ChangeFileExt(FAppName, FDataFileExt);
end;

function TDataLocation.DataFileKey(aDataFileKey: string): IDataLocation;
begin
  Result   := Self;
  FDataKey := aDataFileKey;
end;

function TDataLocation.FindConfigCmd: Boolean;
var
  aFileName : string;
begin
  Result := False;
  if GetCmdLineSwitchValue(aFileName, FConfigCmdParam) then
  begin
    FConfigFile := aFileName;
    Result      := FindLocalConfig;
  end;
end;

function TDataLocation.FindDataCmd: Boolean;
var
  aFileName : string;
begin
  Result := False;
  if GetCmdLineSwitchValue(aFileName, FDataCmdParam) then
  begin
    FDataFile := aFileName;
    Result    := FindLocalFile;
  end;
end;

function TDataLocation.FindLocalConfig: Boolean;
var
  aIniFile : TIniFile;
begin
  Result := False;
  if FileExists(FConfigFile) then
  begin
    aIniFile := TIniFile.Create(FConfigFile);
    try
      FDataFile := aIniFile.ReadString(FConfigSection, FDataKey, FDataFile);
      Result    := FindLocalFile;
    finally
      aIniFile.Free;
    end;
  end;
end;

function TDataLocation.FindLocalFile: Boolean;
begin
  Result := False;
  if FIsLocal then
    Result := FileExists(FDataFile);
end;

function TDataLocation.FindUserDataFile: Boolean;
var
  aDataPath : string;
  aFileName : string;
begin
  Result := False;
  if not FIsUSB then
  begin
    aFileName := ExtractFileName(FDataFile);
    aDataPath := IncludeTrailingBackslash(GetSpecialFolder(CSIDL_APPDATA)) + ChangeFileExt(aFileName, '');
    FDataFile := IncludeTrailingBackslash(aDataPath) + aFileName;
    Result    := FileExists(FDataFile);
  end;
end;

end.

Ein Anwendung müsste dann in etwa so aussehen:

Delphi-Quellcode:

uses
  .. uDataLocation ..

var
  aDataLocation : IDataLocation;
  aDataFile : string;
begin
  aDataLocation := TDataLocation.Create;
  aDataLocation.DataFileExt('.dat')
               .ConfigFile(aDataFile);
  LoadData(aDataFile);

Ein Freigabe von aDataLocation muss nicht explizit erfolgen, weil das Interface von Delphi per RefCounting verwaltet wird.

Mit Verwendung von Spring-DI wird die Einbindung der Unit uDataLocation entfallen und auch TDataLocation.Create; wird durch einen anderen Aufruf ersetzt.

Verbesserung 1: von Furtbichler eingearbeitet, Kommentar von Uwe Raabe in uUtils.pas eingefügt
Verbesserung 2: von DeddyH eingearbeitet
Rückbau zu 1: Hinweis von Furtbichler

[Uwe Raabe]

Zitat:

Zitat von neo4a (Beitrag 1151690)

GetCmdLineSwitchValue()

Mindestens seit XE bietet Sysutils eine überladene Funktion an, die genau das bereits erledigt:

Delphi-Quellcode:

{ This version is used to return values.
  Switch values may be specified in the following ways on the command line:
    -p Value                - clstValueNextParam
    -pValue or -p:Value     - clstValueAppended

  Pass the SwitchTypes parameter to exclude either of these switch types.
  Switch may be 1 or more characters in length. }
function FindCmdLineSwitch(const Switch: string; var Value: string; IgnoreCase: Boolean = True;
  const SwitchTypes: TCmdLineSwitchTypes = [clstValueNextParam, clstValueAppended]): Boolean; overload;

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

Zitat von DeddyH (Beitrag 1151698)

Müsste auf dasselbe herauskommen.

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

[Luckie]

Ich würde als Standarddateiname den Anwendungsnamen ohne Endung nehmen und dem Programmierer zwingen eine Endung anzugeben.

[neo4a]

Zitat:

Zitat von Luckie (Beitrag 1151701)

Ich würde als Standarddateiname den Anwendungsnamen ohne Endung nehmen und dem Programmierer zwingen eine Endung anzugeben.

Kannst Du dafür die Source-Änderung ausführen, damit ich darauf im 1. Post beim Edit verlinken kann? Irgendwie sollte ja der Prozess, das Vorher-Nachher dabei herauskommen.

[Luckie]

Puh, da müsste ich mich jetzt in den Code reinarbeiten. Aber du brauchst ParamStr(0), ChangeFileExt und ExtractFilename.

[neo4a]

Zitat:

Zitat von Luckie (Beitrag 1151704)

Puh, da müsste ich mich jetzt in den Code reinarbeiten. Aber du brauchst ParamStr(0), ChangeFileExt und ExtractFilename.

Ja, das sind aktuell 187 Zeilen ;) Wie genau stellst Du Dir das "Zwingen" über das Interface vor?

Üblicherweise würde man das wohl über den Konstruktor machen. Den wollte ich aber eigentlich so simpel belassen, damit ich dann bei der Umstellung für Spring-DI nicht gleich mit DelegateTo()-Aufrufen hantieren muss.

[Luckie]

Na ja, damit die Konfigurationsdatei oder was auch immer nicht ohne Endung im Dateisystem steht.

[Furtbichler]

Zitat:

Zitat von neo4a (Beitrag 1151700)

Zitat:

Zitat von DeddyH (Beitrag 1151698)

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:

Zitat von Furtbichler (Beitrag 1151708)

Zitat:

Zitat von neo4a (Beitrag 1151700)

Zitat:

Zitat von DeddyH (Beitrag 1151698)

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:

Zitat von Furtbichler (Beitrag 1151712)

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:

Zitat von Furtbichler (Beitrag 1151708)

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

Delphi-Quellcode:

{$IFOPT B+}

und wirft einen Compilerfehler.

Was die Auswertereihenfolge betrifft: Ein Compiler, der ein Konstrukt wie

Delphi-Quellcode:

if (P<> nil) and P.Enabled then

in eine Schutzverletzung führt, hat bei m.E. sowieso keine Chancen.

[himitsu]

Delphi-Quellcode:

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. :stupid:

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

Zitat von Uwe Raabe (Beitrag 1151715)

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

Zitat von Uwe Raabe (Beitrag 1151715)

Was die Auswertereihenfolge betrifft: Ein Compiler, der ein Konstrukt wie

Delphi-Quellcode:

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:

Zitat von himitsu (Beitrag 1151714)

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:

Zitat von himitsu (Beitrag 1151717)

Delphi-Quellcode:

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. :stupid:

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:

Zitat von Furtbichler (Beitrag 1151719)

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. :roll:

Denn mit voller booleanischen Auswertung kann der Code nur in

Delphi-Quellcode:

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:

Zitat von Furtbichler (Beitrag 1151721)

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:

Delphi-Quellcode:

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.

[neo4a]

Zitat:

Zitat von himitsu (Beitrag 1151714)

Und ich bin mir fast sicher, daß selbst CleanCode was dagegen hätte.

So, wie Du das hier formulierst, müsste es eine Frau sein ;) (CC = Coco Chanel)

Aber im Ernst: Genauso wie bei der Code-Formatierung sollte auch der Programmfluß keine "Mätzchen" machen. D.h., nur weil etwas funktioniert/kompiliert, muss man es noch lange nicht machen.

Ich hatte bislang Exceptions auch eher als Ausnahmen eingesetzt und im finally/except-Block nie funktionalen Code hinterlegt. Also war meine erste Reaktion bei Furtbichlers Code: Und wo wird nun eigentlich der out-Parameter belegt?

Andererseits wird in anderen Sprachen sehr offensiv mit Exceptions gearbeitet und das nicht nur in Ausnahmefällen. Vielleicht Zeit für mich, auch hier etwas umzudenken.

[himitsu]

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

Delphi-Quellcode:

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

Und jetzt debugge mal ein Programm, wo Sowas oder Ähnliches in jeder zweiten Codezeile vorkommt.
Das macht absolut keinen Spaß mehr, wenn der Debugger ständig anhält.

Die Exceptions zu ignorieren ist auch keine Lösung, denn dann werden vom Debugger auch Exceptions ignoriert, welche man gerne erfahren würde.

[neo4a]

Zitat:

Zitat von Furtbichler (Beitrag 1151712)

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.

[neo4a]

Zitat:

Zitat von himitsu (Beitrag 1151729)

Größtes Gegenargument gegen Exceptions zur regulären Programmsteuerung.
...
Das macht absolut keinen Spaß mehr, wenn der Debugger ständig anhält.

Die Exceptions zu ignorieren ist auch keine Lösung, denn dann werden vom Debugger auch Exceptions ignoriert, welche man gerne erfahren würde.

Was spricht gegen spezielle Exception-Klassen?

[Uwe Raabe]

Zitat:

Zitat von neo4a (Beitrag 1151731)

Zitat:

Zitat von himitsu (Beitrag 1151729)

Größtes Gegenargument gegen Exceptions zur regulären Programmsteuerung.
...
Das macht absolut keinen Spaß mehr, wenn der Debugger ständig anhält.

Die Exceptions zu ignorieren ist auch keine Lösung, denn dann werden vom Debugger auch Exceptions ignoriert, welche man gerne erfahren würde.

Was spricht gegen spezielle Exception-Klassen?

In diesem Fall wäre das wohl EConvertError und den schaltet man besser nicht generell ab.

[neo4a]

Zitat:

Zitat von Uwe Raabe (Beitrag 1151732)

Zitat:

Zitat von neo4a (Beitrag 1151731)

Zitat:

Zitat von himitsu (Beitrag 1151729)

Größtes Gegenargument gegen Exceptions zur regulären Programmsteuerung.
...
Das macht absolut keinen Spaß mehr, wenn der Debugger ständig anhält.

Die Exceptions zu ignorieren ist auch keine Lösung, denn dann werden vom Debugger auch Exceptions ignoriert, welche man gerne erfahren würde.

Was spricht gegen spezielle Exception-Klassen?

In diesem Fall wäre das wohl EConvertError und den schaltet man besser nicht generell ab.

Schon klar. Aber ich hatte mich nicht so sehr auf das nicht gequotete Beispiel bezogen als vielmehr auf eine Möglichkeit, Exceptions zur Programmsteuerung zu nutzen ohne sich ständigen Debugger-Halts auszusetzen.

[Furtbichler]

Zitat:

Zitat von himitsu (Beitrag 1151722)

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

Delphi-Quellcode:

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:

Zitat von himitsu (Beitrag 1151729)

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:

Delphi-Quellcode:

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

Delphi-Quellcode:

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:

Zitat von Stevie (Beitrag 1151975)

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.

[Stevie]

Zitat:

Zitat von Furtbichler (Beitrag 1152009)

Zitat:

Zitat von Stevie (Beitrag 1151975)

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

Bibel welcher Art war schon immer Auslegungssache ;)

[Furtbichler]

Zitat:

Zitat von Stevie (Beitrag 1152026)

Bibel welcher Art war schon immer Auslegungssache ;)

Die wird von den Jüngern halt so genannt.