Wie ist der Programmierstil "professionell"

[coderi]

Hallo Zusammen,

ich habe hier mal eine Grundsatzfrage zum Thema Programmieren in Delphi

Delphi-Quellcode:

procedure TfrmXXX.ZeileDelExecute(Sender: TObject);
begin

   dmXXX.qGETFertigteileeingang.ParamByName('AZID').AsInteger := dmXXX.AZQ.fieldbyname('AZ.ID').asinteger;
   dmXXX.qGETFertigteileeingang.Open;
   if (dmXXX.qGETFertigteileeingang.FieldByName('Anzahl').AsInteger > 0) then
   begin
      MessageDlg('Löschen der Auftragszeile NICHT möglich, da bereits Fertigteile daraus eingebucht wurden!', mtError, [mbOK], 0);
      dmXXX.qGETFertigteileeingang.close;
      exit;
   end;
   dmXXX.qGETFertigteileeingang.close;
    .
. .
.. 
code hier noch nicht zu ende.

Erklärung:

Es geht hier um einen Rowcount mit anschließender Auswertung, wenn die Anzahl > 0 dann soll er nach Meldung direkt abbrechen und die Procedure verlassen.
Die query möchte ich aber immer geschlossen wissen.

Frage:
Unter Berücksichtigung von gutem Programmierstyle, Codewartbarkeit u.a. :

Ist es so korrekt wie oben dargestellt?

Oder benutze ich eine Variable als Zwischenspeicher wie hier:

Delphi-Quellcode:

procedure TfrmAuftrag.ZeileDelExecute(Sender: TObject);
var
 azid:Integer;
begin

   dmAuftrag.qGETFertigteileeingang.ParamByName('AZID').AsInteger := dmauftrag.AZQ.fieldbyname('AZ.ID').asinteger;
   dmAuftrag.qGETFertigteileeingang.Open;
   azid := dmAuftrag.qGETFertigteileeingang.FieldByName('Anzahl').AsInteger;
   dmAuftrag.qGETFertigteileeingang.close;
   if (azid > 0) then
   begin
      MessageDlg('Löschen der Auftragszeile NICHT möglich, da bereits Fertigteile daraus eingebucht wurden!', mtError, [mbOK], 0);
      exit;
   end;

Oder gib es einen ganz anderen Ansatz dafür?

Antworte bitte mit Begründung

danke euch
grüße
Coderi

[Bjoerk]

Die Extravariable ist wohl eher Geschmackssache, aber Exit verwende ich persönlich nur nur, wenn es gar nicht anders geht, dafür aber öfter mal ein try .. finally.

Delphi-Quellcode:

   dmXXX.qGETFertigteileeingang.ParamByName('AZID').AsInteger := dmXXX.AZQ.fieldbyname('AZ.ID').asinteger;
   dmXXX.qGETFertigteileeingang.Open;
   try
     if (dmXXX.qGETFertigteileeingang.FieldByName('Anzahl').AsInteger > 0) then
       MessageDlg(' .. ', mtError, [mbOK], 0)
     else
     begin

     end;
   finally
     dmXXX.qGETFertigteileeingang.close;
   end;

[Stevie]

Ob nun Variable, Exit oder nicht - mir fällt da was ganz anderes auf in Hinsicht auf Wartbarkeit. Es handelt um etwas, was mit dem Dataset gemacht wird. Dennoch ist diese Logik scheinbar in einem Form oder Frame implementiert - warum nicht im Datamodule?

In der jetzigen Form kannst du das Datamodule nicht separat einsetzen bzw testen, es funktioniert nur in Verbindung mit diesem Form/Frame.

[p80286]

Um Stevies Bemerkung aufzunehmen, besser wäre wohl so etwas:

Delphi-Quellcode:

------
Datamodule:
function Isrecordused(recnr:integer):boolean;
....
result:=dmAuftrag.qGETFertigteileeingang.FieldByName('Anzahl').AsInteger>0;
...
end;

function Delete(recnr);
....
end;
-----------------
Hauptprogramm:
if not(IsrecordUsed(recnr) then delete(recnr);

Wenn du dich an dieses Prinzip hälst kannst Du Problemlos die DB-Schnittstelle austauschen, ohne am eigentlichen Programm eine Änderung durchzuführen.
(Namen sind Schall und Rauch! das Delete ist nicht der Weisheit letzter Schluß!)

Gruß
K-H

[Sir Rufo]

Zitat von Bjoerk:

Die Extravariable ist wohl eher Geschmackssache, aber Exit verwende ich persönlich nur nur, wenn es gar nicht anders geht, dafür aber öfter mal ein try .. finally.

Bei deinem Konstrukt kann man getrost Exit verwenden, weil der finally Block immer ausgeführt wird.

Delphi-Quellcode:

procedure SomeStuff;
begin
  SomeData.Open;
  try // Wenn man hier drüber weg ist, dann ...
    ...
    if SomeThing then
      Exit;
    ...
  finally // ... wird dieser Block immer ausgeführt
    SomeData.Close;
  end;
end;

[sx2008]

Wenn's "professionell" sein soll, dann muss man die Businesslogik extrahieren und entsprechend abbilden.
Wenn man viele Anweisung hat, die mehr als einen Punkt enthalten, dann müssen die Alarmglocken klingeln:

Delphi-Quellcode:

procedure TfrmXXX.ZeileDelExecute(Sender: TObject);
begin
   // wir befinden uns in der Klasse TfrmXXX
   // und trotzdem greifen wir immer wieder auf die internen Dinge des Datenmoduls
   // TdmXXX zu
   dmXXX.qGETFertigteileeingang.ParamByName('AZID').AsInteger := dmXXX.AZQ.fieldbyname('AZ.ID').asinteger;
   // und hier schon wieder - 2. Punkte in einer Anweisung
   // das "stinkt"
   dmXXX.qGETFertigteileeingang.Open;

Also es ist klar, der Code zum Auslesen der Anzahl der Fertigteile gehört in das Datenmodul

Delphi-Quellcode:

function TdmXXX.GetFertigteileAnzahl(azid : Integer):integer;
begin
   qGETFertigteileeingang.ParamByName('AZID').AsInteger := azid;
   qGETFertigteileeingang.Open;
   result := qGETFertigteileeingang.FieldByName('Anzahl').AsInteger;
   qGETFertigteileeingang.close;
end;

Und jetzt zum 2. Problem:
Es wird MessageDlg() verwendet wo eigentlich eine Exception ausgelöst werden sollte:

Delphi-Quellcode:

procedure TfrmXXX.ZeileDelExecute(Sender: TObject);
var
  anzahl : Integer;
  azid : Integer;
begin
  azid := dmXXX.AZQ.fieldbyname('AZ.ID').asinteger;
  anzahl := dmXXX.GetFertigteileAnzahl(azid);
  if (anzahl > 0) then
    // Exception auslösen und dem Benutzer möglichst genaue Info geben wo das Problem ist
    raise Exception.CreateFmt('Löschen der Auftragszeile NICHT möglich, da bereits %d Fertigteile daraus eingebucht wurden (azid=%d)', [anzahl, azid]);

[Stevie]

Zitat von sx2008:

Und jetzt zum 2. Problem:
Es wird MessageDlg() verwendet wo eigentlich eine Exception ausgelöst werden sollte:

Delphi-Quellcode:

procedure TfrmXXX.ZeileDelExecute(Sender: TObject);
var
  anzahl : Integer;
  azid : Integer;
begin
  azid := dmXXX.AZQ.fieldbyname('AZ.ID').asinteger;
  anzahl := dmXXX.GetFertigteileAnzahl(azid);
  if (anzahl > 0) then
    // Exception auslösen und dem Benutzer möglichst genaue Info geben wo das Problem ist
    raise Exception.CreateFmt('Löschen der Auftragszeile NICHT möglich, da bereits %d Fertigteile daraus eingebucht wurden (azid=%d)', [anzahl, azid]);

Und selbst diese Methode gehört ins DM. Dann merkt man nämlich auch, worin der Vorteil der Exception gegenüber dem MessageDlg liegt: man hat keine GUI im DM. Wobei man sich darüber streiten kann, ob hier nicht lieber ein Result vom Typ Boolean einer Exception vorzuziehen wäre (Stichwort: keine Exceptions als Teil der Business Logic).

[Sir Rufo]

Und um nicht in Versuchung zu kommen, kann man dem DataModule auch ein Interface verpassen und der Form nur dieses Interface übergeben. Dann ist Schluss mit dem wilden Zugreifen auf die Komponenten

[sx2008]

Zitat von Sir Rufo:

...kann man dem DataModule auch ein Interface verpassen

Aber man würde auch einen Preis dafür bezahlen.
1.) mehr Sourcecode
2.) falls man eine Methode ändern möchte, muss man an mindestens 3 Stellen ändern
im Interface, in der Datenmodul-Klasse und beim Aufrufer

Durch den zusätzlichen Aufwand auch noch das Interface zu ändern, werden einige Programmierer notwendige Änderungen unterlassen und sich irgendwie durchmogeln.
Vielleicht wird dann zum Teil über das Interface und daneben auch direkt auf das Datenmodul zugegriffen (was natürlich sehr schlecht wäre).

Interfaces haben eben ihre Vor- und Nachteile.

[Lemmy]

Zitat von coderi:

Frage:
Unter Berücksichtigung von gutem Programmierstyle, Codewartbarkeit u.a. :

Ist es so korrekt wie oben dargestellt?

Oder benutze ich eine Variable als Zwischenspeicher wie hier:

Steve McConnell beschreibt die Version mit der Variable als Zwischenspeicher als "geschwätzigen Code". Es besteht zudem die Gefahr, dass bei einer späteren Erweiterung des Codes die Versuchung nahe liegt die Variable für was anderes zu verwenden.

Es KÖNNTE jedoch hilfreich sein, eine Variable mit einem starken Namen einzuführen, damit diese den Code besser dokumentiert als so ein Monsterausdruck.


Alles in allem also eher eine Frage des Geschmacks, der Situation und der Begleitumstände.