Constructor Rückgabewert nil-en statt Instanz zurückzugeben?

[günni0]

In einem Konstruktor einer Klasse verarbeite ich eine Stringvariable. Wenn der Inhalt in irgendeiner Art und Weise korrupt ist, wird im Constructor nichts weiter verarbeitet.

Das Problem ist, dass ich diese Klasseninstanz beim Erzeugen in eine generische TObjectList<> packe:

Delphi-Quellcode:

GenerischeTObjectList.Add(Klasse.Create(Parameter));

Wenn im Konstruktor nun wegen korrupter Daten keine weiteren Daten vorbereitet werden, soll ein Schlussstrich gezogen werden und diese Instanz nicht in die generische TObjectList gepackt werden.

Wie bewerkstellige ich das?

[freimatz]

Im constructor eine exception raisen und außen dann entsprechend behandeln.
Oder das ganze Design überarbeiten. Solche Dinge gehören eigentlich nicht in einen Konstruktor.

[günni0]

Es ist nur ein String der dort verarbeitet wird. Wird so schlimm nicht sein ;)

Ansonsten hätte ich diese Verarbeitung bzw. einen Prozeduraufruf doppelt, da ich zwei Stellen habe wo ich den Constructor aufrufe.

Ist das hier ausreichend (Code ist nur zum Verständnis)

Delphi-Quellcode:

try
 InstanceX := ClassX.Create(); // raise exception im Constructor mit Nachricht für mein Log-File
except
end;

if Assigned(InstanceX) then
 GenerischeListe.Add(InstanceX);

[himitsu]

Kein Constructor, sondern eine Class-Function, die auch gern CreateIrgendwas heißen darf.
Da drin kannst du dann machen was du willst.

[Stevie]

Arbeit im Konstruktor ist ein Anti Pattern

[bernau]

Zitat:

Zitat von Stevie (Beitrag 1398756)

Arbeit im Konstruktor ist ein Anti Pattern

Wäre dann TFilestream.create ein Antipattern? dort wird ja auch im Constructor die Datei geöffnet.

[bernau]

Zitat:

Zitat von himitsu (Beitrag 1398751)

Kein Constructor, sondern eine Class-Function, die auch gern CreateIrgendwas heißen darf.
Da drin kannst du dann machen was du willst.

So würde ich das auch machen :thumb:

bzw. ich mache das so öfters.

[Stevie]

Zitat:

Zitat von bernau (Beitrag 1398760)

Zitat:

Zitat von Stevie (Beitrag 1398756)

Arbeit im Konstruktor ist ein Anti Pattern

Wäre dann TFilestream.create ein Antipattern? dort wird ja auch im Constructor die Datei geöffnet.

Ein TFileStream klatscht dir mit einer Exception um die Ohren, wenn die Datei aus welchen Gründen auch immer nicht geöffnet werden kann und überlässt dir nicht nil oder eine unbrauchbare Instanz - und das ist dann auch eine Ausnahme.

Da ich nicht der "niemals/immer" Typ bin, bin ich persönlich keineswegs gegen alles außer Zuweisungen im Konstruktor. Wenn das, was dort geschieht zur Objektinitialisierung dient - und ein TFileStream ist ja nunmal dafür da (gut, man könnte jetzt argumentieren, dass das eigentliche Öffnen in einer Open Methode geschehen sollte), dann lass ich das durchaus zu.

Davon abgesehen ist die RTL und VCL aber keineswegs das Maß der Dinge, wenn es um guten Stil oder Befolgung von Patterns geht ;)

[günni0]

Zitat:

Zitat von himitsu (Beitrag 1398751)

Kein Constructor, sondern eine Class-Function, die auch gern CreateIrgendwas heißen darf.
Da drin kannst du dann machen was du willst.

So einfach ist das nicht.
Das ist eine Klasse deren Konstruktor entweder einen Thread A oder Thread B erzeugt. Thread A hat zudem ein Notify-Event als Parameter.

Und dann kommt noch hinzu, dass meine generische Liste Instanzen dieser Klasse aufnimmt.
Einfach hier und da was ändern is nich.

[bytecook]

Zitat:

Zitat von Stevie (Beitrag 1398763)

Zitat:

Zitat von bernau (Beitrag 1398760)

Zitat:

Zitat von Stevie (Beitrag 1398756)

Arbeit im Konstruktor ist ein Anti Pattern

Wäre dann TFilestream.create ein Antipattern? dort wird ja auch im Constructor die Datei geöffnet.

Ein TFileStream klatscht dir mit einer Exception um die Ohren, wenn die Datei aus welchen Gründen auch immer nicht geöffnet werden kann und überlässt dir nicht nil oder eine unbrauchbare Instanz - und das ist dann auch eine Ausnahme.

Da ich nicht der "niemals/immer" Typ bin, bin ich persönlich keineswegs gegen alles außer Zuweisungen im Konstruktor. Wenn das, was dort geschieht zur Objektinitialisierung dient - und ein TFileStream ist ja nunmal dafür da (gut, man könnte jetzt argumentieren, dass das eigentliche Öffnen in einer Open Methode geschehen sollte), dann lass ich das durchaus zu.

Davon abgesehen ist die RTL und VCL aber keineswegs das Maß der Dinge, wenn es um guten Stil oder Befolgung von Patterns geht ;)

This!

Eventuell auch in diesem Zusammenhang für den Threadersteller interessant: Dependency Injections von Nick Hodges (https://leanpub.com/dependencyinjectionindelphi)
(Stevie brauchts hws nicht mehr lesen :P)

[günni0]

Schön zu lesen, dass sich nicht alle unbedingt an irgendwelche Vorgaben halten die irgendjemand irgendwann mal in die Welt geworfen hat.
Was in meinem Konstruktor passiert sind kleinere Überprüfungen, Stringmanipulationen und das Erzeugen eines Threads. Mehr nicht.

Dafür, um das jetzt schön sauber zu bekommen, zerbreche ich mir jetzt nicht Tage lang den Kopf.

[DevLinus]

Wie wäre es mit einer Funktion, die man dazwischen Schaltet. Sprich eine Funktion die den String überprüft und wenn der String Valied ist wird der Constructor aufgerufen und wenn nicht gibt die Funktion Nil zurück.

Ich bin ein Fan davon nur richtige Werte an einen Constructor zu übergeben und mögliche "Fehlerquellen" vorher auszuschließen.

[günni0]

Ich versuche es mal.

Am liebsten wäre es mir natürlich, wenn meine ObjectList Records (mit class vars und class functions/procedures) aufnehmen könnte statt Klasseninstanzen. Dann hätte ich das ganze Problem erst gar nicht.

[Uwe Raabe]

Zitat:

Zitat von günni0 (Beitrag 1398782)

Am liebsten wäre es mir natürlich, wenn meine ObjectList Records (mit class vars und class functions/procedures) aufnehmen könnte statt Klasseninstanzen.

Und warum nimmst du dann nicht eine TList<TMyRecord>?

[günni0]

Ich versuche es heute Nachmittag mal.
Schade nur, dass ich dann den Destructor verliere, oder ist dem nicht so?

Eigentlich bin ich ganz zufrieden so wie es jetzt ist.

[himitsu]

warum Destructor verlieren?

In der Class-Function mußt du natürlich auch einmal den Constructor ausführen, um die Instanz zu erstellen, und bei deren Freigabe kommt dann der Destructor.
Aber du kannst in der Class-Funktion beim Fehler entweder die Anstanz garnicht erst erstellen oder sie gleich wieder freigeben und dann eben dein NIL zurück liefern.

[Blup]

Eigentlich gibt es doch nur diese beiden Grund-Varianten, abhängig davon wie der weitere Programmablauf sein soll:

Delphi-Quellcode:

procedure ErzeugeObjekte(AParams: TStrings);
var
  s: string;

{Vartiante1}
begin
  while s in APrams do
  begin
    try
      GenerischeTObjectList.Add(Klasse.Create(s));
    except
      {Excpetion loggen oder so}
    end;
  end;
end;

{Vartiante2}
begin
  try
    while s in APrams do
    begin
      GenerischeTObjectList.Add(Klasse.Create(s));
    end;
  except
    {Excpetion loggen oder so}
  end;
end;

In beiden Varianten wird im Fehlerfall die Liste nicht erweitert. Genau für solche Fälle ist eine Exception im Constuctor doch die richtige Lösung.

[jaenicke]

Eine einfache Variante wäre eine Factory-Klassenmethode TKlasse.TryGetInstance, dass die passenden Parameter bekommt. Nur, wenn diese erfolgreich zurückgibt, benutzt du die Instanz und fügst sie in die Liste hinzu. Dann kannst du auch die String Analyse aus dem Constructor herauslösen, weil es ja ohnehin in der Klassenmethode gekapselt ist.

[günni0]

Wie würde man so eine Factory-Klasse auf meinen Fall aufbauen?

[freimatz]

Zitat:

Zitat von günni0 (Beitrag 1398769)

... und das Erzeugen eines Threads. Mehr nicht.

Der war gut :bounce1:

[günni0]

Ich gebe mal etwas vor. Wie sähe das denn korrekt aus mit einer Factory-Class?

Delphi-Quellcode:

type
 TMyClass = class
 public
  constructor Create(Parameter);
  destructor Destroy; override;
end;

implementation

constructor TMyClass .Create(Parameter);
begin
 prüfe dies

 prüfe das

 WENN Fehler/korrupte Daten, dann raise exception und abbrechen

 SONST
  am Ende erstelle Thread -A- oder -B-
end;

destructor TMyClass .Destroy;
begin
 if Assigned(ThreadA) then
  begin
   ThreadA.Stop; // Funktion in der Thread-Unit selber - hat schon seinen Sinn :P
   ThreadA.Free;
  end;

  // ThreadB, wenn Assigned, wird an anderer Stelle von Hand freigegeben

 inherited Destroy;
end;

Delphi-Quellcode:

TMyClass: TList;


try
 myClass := TMyClass.Create(Parameter);
except
end;

if Assigned(myClass) then // wenn oben rause ausgelöst wird, ist myClass not Assigned und muss nicht hinzugefügt werden
 MyGenericTList.Add(myClass);

[Uwe Raabe]

Warum nicht so?

Delphi-Quellcode:

try
  myClass := TMyClass.Create(Parameter);
  MyGenericTList.Add(myClass);
except
  // hier sollte schon och was passieren, oder?
end;

[Stevie]

Da du im Konstruktor bist und als input also nur "parameter" zur Verfügung hast und nicht irgendwelchen State einer schon vorhandenen Instanz frage ich mich, warum "prüfe dies" und "prüfe das" dort drin sein muss und nicht vorgelagert, so dass eine Instanzerstellung erst dann durchgeführt werden muss, wenn auch alles i.O ist? Aber das hängt davon ab, wie häufig nun dieser Ausnahmefall auftritt.

Wenn das in der gleichen Kategorie wie "Datei beim Erstellen eines TFileStreams konnte nicht geöffnet werden" anzuordnen ist, dann ist das mMn schon in Ordnung. Ablauf dann so wie Uwe schon schrieb.

[günni0]

Zitat:

Warum nicht so?

...

Werde ich gleich testen.

Zitat:

Da du im Konstruktor bist und als input also nur "parameter" zur Verfügung hast und nicht irgendwelchen State einer schon vorhandenen Instanz frage ich mich, warum "prüfe dies" und "prüfe das" dort drin sein muss und nicht vorgelagert, so dass eine Instanzerstellung erst dann durchgeführt werden muss, wenn auch alles i.O ist?

Wenn ich das alles auslagere habe ich an zwei Stellen einen Aufruf mehr. Davon geht die Welt nicht unter. Ich werde es testen.
Dann wäre ich zudem auch das try-except los.
Zum Schluss könnte ich, da ich den Aufruf zweimal habe, das alles in eine Prozedur packen. Dann muss ich nur an einer Stelle ändern, wenn es was zu ändern gibt.
Ist schluss-endlich alles viel einfacher als Factory-Konstrukte o.ä.

Edit
klappt soweit ganz gut. Code ist sauber, im Konstruktor wird nur noch den Properties-Variablen zugewiesen und alle Überprüfungen sind raus.

[jaenicke]

Zur Ergänzung noch die Lösung per Factory-Methode:

Delphi-Quellcode:

TMyClass = class
public
  class function TryGetInstance(const AValue: string; out AInstance: TMyClass): Boolean;
end;

class function TMyClass.TryGetInstance(const AValue: string; out AInstance: TMyClass): Boolean;
var
  NewInstance: TMyClass;
begin
  NewInstance := TMyClass.Create;
  Result := NewInstance.Init(AValue);
  if Result then
    AInstance := NewInstance
  else
    NewInstance.Free;
end;

Vorteil ist, dass du zur Verwendung nur brauchst:

Delphi-Quellcode:

var
  MyObject: TMyClass;
begin
  if TMyClass.TryGetInstance('blub', MyObject) then
    List.Add(MyObject);

Besser wäre natürlich, wenn die Prüfung wie schon genannt wurde auch z.B. per Klassenmethode möglich wäre, dann müsste bei einem Fehler gar nicht erst eine dann nicht benötigte Instanz erstellt werden.

[günni0]

Zitat:

Besser wäre natürlich, wenn die Prüfung wie schon genannt wurde auch z.B. per Klassenmethode möglich wäre, dann müsste bei einem Fehler gar nicht erst eine dann nicht benötigte Instanz erstellt werden.

Deswegen habe ich es nach euren Vorschlägen auch so umgesetzt.

Ich habe den Code ordentlich in verschiedene Units aufgeteilt.
Erst rufe ich eine Prozedur aus der ersten Unit auf. Die überprüft alle Daten und wenn alles OK ist, ruft Prozedur 1 den Constructor in einer anderen Prozedur auf.

Genau eine Zeile brauche ich für diesen Aufruf.