Saubere Programmierung mit Delphi

[Christian18]

Hallo,

da ich sehr viel Wert auf saubere Programmierung lege, habe ich mir folgendes gedacht. Vieleicht könntet ihr euch mal eine Klasse von mir anschauen und mir ein paar Tips geben, was ich vieleicht besser machen kann.

Delphi-Quellcode:

unit UBackground;

interface

uses
  Forms, Graphics;

type
  TBackground = class
  private
    FForm     : TForm;
    FColor    : Array[0..1] of TColor;
    FDistance : Byte;
  public
    constructor Create(Form: TForm; Color1, Color2: TColor; Distance: Byte);
    procedure Horizontal();
    procedure Vertical();    
    destructor Destroy();
  end;

implementation

constructor TBackground.Create(Form: TForm; Color1, Color2: TColor; Distance: Byte);
begin
  FForm:=Form;
  FColor[0]:=Color1;
  FColor[1]:=Color2;
  FDistance:=Distance;
end;

procedure TBackground.Horizontal();
  var i : Integer;  // Schleifenvariable
begin
  FForm.Color:=FColor[0];
  FForm.Canvas.Pen.Color:=FColor[1];
  i:=0;
  while i <= FForm.ClientHeight do
    begin
      FForm.Canvas.MoveTo(0, i);
      FForm.Canvas.LineTo(FForm.ClientWidth, i);
      Inc(i, FDistance);
    end;
end;

procedure TBackground.Vertical();
  var i : Integer;  // Schleifenvariable
begin
  FForm.Color:=FColor[0];
  FForm.Canvas.Pen.Color:=FColor[1];
  i:=0;
  while i <= FForm.ClientWidth do
    begin
      FForm.Canvas.MoveTo(i, 0);
      FForm.Canvas.LineTo(i, FForm.ClientHeight);
      Inc(i, FDistance);
    end;
end;

destructor TBackground.Destroy();
begin
  FForm.Free();
end;

end.

MFG Christian18

[Larsi]

Ich weiß jetzt nicht genau was du meinst? Willst du, dass dir jemand sagt, dass xyz bei abc an der Stelle besser aussieht oder etwas was vielleicht Code technisch einen Vorteil hat?

[mkinzler]

Es fehlt das inherited beim Destruktor

[omata]

Wieso gibt du FForm wieder frei? Das Objekt wird doch übergeben?

Wenn ich dort nil übergebe, schützt du deine interne Verarbeitung nicht.

Gruss
Thorsten

[Christian18]

Brauche ich denn überhaupt einen Destructor?

[omata]

Zur Zeit ja, weil du ja deine FForm dort freigibst.

Und den deklariert man dann als override.

[mkinzler]

Eigentlich nicht.

[Christian18]

in deinem ersten beitrag klang es so, als müsste ich die form gar nicht freigeben. also könte ich dann den dectructor auch weg lassen.

LG Chris

[1234588]

FForm wird ja normalerweise mit der TBackground instance zerstoert, imho

[Christian18]

keine ahnung. würde mich aber freuen, wenn mir jemand ein paar tips gibt wie ich es besser machen kann?

[mkinzler]

Zitat:

Zitat von 1234588

FForm wird ja normalerweise mit der TBackground instance zerstoert, imho

Wenn das Objekt auch dort erzeugt wurde. was hier aber nicht der Fall ist.

[Christian S.]

Ich finde es merkwürdig, dass TBackground die Form zerstört, obwohl es sie nicht erzeugt. IMHO sollte nur derjenige, der etwas erzeugt, auch für das Zerstören zuständig sein.

Außerdem solltest Du Dir hier mal die Konvention zur Methoden-Benennung angucken.

[omata]

Hier mal ein Vorschlag...

Delphi-Quellcode:

unit UBackground;

interface

uses
  Forms, Graphics;

type
  TBackground = class
  private
    FForm     : TForm;
    FColor    : Array[0..1] of TColor;
    FDistance : Byte;
  public
    constructor Create(AForm: TForm; AColor1, AColor2: TColor; ADistance: Byte);
    procedure Horizontal;
    procedure Vertical;   
  end;

implementation

constructor TBackground.Create(AForm: TForm; AColor1, AColor2: TColor; ADistance: Byte);
begin
  FForm:=AForm;
  FColor[0]:=AColor1;
  FColor[1]:=AColor2;
  FDistance:=ADistance;
end;

procedure TBackground.Horizontal;
var i : Integer;  // Schleifenvariable
begin
  if assigned(FForm) then begin
    FForm.Color:=FColor[0];
    FForm.Canvas.Pen.Color:=FColor[1];
    i:=0;
    while i <= FForm.ClientHeight do begin
      FForm.Canvas.MoveTo(0, i);
      FForm.Canvas.LineTo(FForm.ClientWidth, i);
      Inc(i, FDistance);
    end;
  end;
end;

procedure TBackground.Vertical;
var i : Integer;  // Schleifenvariable
begin
  if assigned(FForm) then begin
    FForm.Color:=FColor[0];
    FForm.Canvas.Pen.Color:=FColor[1];
    i:=0;
    while i <= FForm.ClientWidth do begin
      FForm.Canvas.MoveTo(i, 0);
      FForm.Canvas.LineTo(i, FForm.ClientHeight);
      Inc(i, FDistance);
    end;
  end;
end;

end.

Gruss
Thorsten

[semo]

parameter sollten als const übergeben werden in deinem constructor

[Union]

Du setzt die Farbwerte nicht auf den Ursprungszustand zurück. Wenn jemand Deine Klasse benutzt und selber Zeichenoperationen auf dem Canvas durchführt, änderst Du einfach die Farben.

Vor den Schleifen solltest Du FForm.ClientWidth und FForm.ClientHeight in einer Integervariable speichern. Dies ist schneller als der Zugriff über die Formproperties in der Schleife.

[Muetze1]

Zitat:

Zitat von Union

Du setzt die Farbwerte nicht auf den Ursprungszustand zurück. Wenn jemand Deine Klasse benutzt und selber Zeichenoperationen auf dem Canvas durchführt, änderst Du einfach die Farben.

Das heisst du initialisierst niemals deine Farben vor Zeichenoperationen? Ich finde das vergebliche Mühe, da andere Zeichenoperationen fast immer die Farbe vor derer selber festlegen bzw. sogar unterschiedliche, von anderen Parametern abhängige Farben, festlegen.

Zitat:

Zitat von M. Hassmann

parameter sollten als const übergeben werden in deinem constructor

Und lieferst du dazu auch eine Begründung?

Und nun noch mein Vorschlag:

Delphi-Quellcode:

unit UBackground;

interface

uses
  Forms, Graphics;

type
  TBackground = class
  private
    fBackgroundColor: TColor;
    fLineColor: TColor;
    fDistance: Integer;
  public
    procedure Horizontal(AForm: TForm);
    procedure Vertical(AForm: TForm);   

    property BackgroundColor: TColor read fBackgroundColor write fBackgroundColor;
    property LineColor: TColor read fLineColor write fLineColor;
    property Distance: integer read fDistance write fDistance;
  end;

implementation

procedure TBackground.Horizontal(AForm: TForm);
var 
  lWidth, lHeight: integer;
begin
  if assigned(AForm) then 
  begin
    AForm.Color := fBackgroundColor;
    AForm.Canvas.Pen.Color := fLineColor;

    lWidth := AForm.ClientWidth;
    lHeight := AForm.ClientHeight;
    while lHeight >= 0 do 
    begin
      AForm.Canvas.MoveTo(0, lHeight);
      AForm.Canvas.LineTo(lWidth, lHeight);

      Dec(lWidth, fDistance);
    end;
  end;
end;

procedure TBackground.Vertical(AForm: TForm);
var 
  lWidth, lHeight: Integer;  
begin
  if assigned(AForm) then 
  begin
    AForm.Color := fBackgroundColor;
    AForm.Canvas.Pen.Color := fLineColor;

    lHeight := AForm.ClientHeight;
    lWidth := AForm.ClientWidth;
    while lWidth >= 0 do 
    begin
      AForm.Canvas.MoveTo(lWidth, 0);
      AForm.Canvas.LineTo(lWidth, lHeight);

      Dec(lWidth, fDistance);
    end;
  end;
end;

end.

[Christian18]

Gutem Morgen,

Muetze1, ich habe da mal ne Frage. Warum machst du alle Eigenschaften der Klasse TBackground über property öffentlcih und die Form nicht?

MFG Christian18

[Phoenix]

Zitat:

Zitat von Christian18

Delphi-Quellcode:

  TBackground = class
  private
    FForm     : TForm;
    FColor    : Array[0..1] of TColor;
    FDistance : Byte;

Kleiner Tipp aus der Praxis:

Delphi-Quellcode:

  TBackground = class
  private
    FForm: TForm;
    FColor: Array[0..1] of TColor;
    FDistance: Byte;

Ergo: Keine (oder immer eine feste Anzahl an) Leerzeichen zwischen Name und Doppelpunkt, und dann der Typ.

Des öfteren Sucht man eine Deklaration um schnell dort hin zu gelangen. Ich würde dann z.B. immer nach "FColor:" suchen. Wenn da Leerzeichen zwischen sind findet die Suche das nicht. Und nur nach FColor suchen zeigt ja nicht nur die Deklaration, sondern jedes Vorkommen an. Ist also nicht das gewünschte Ergebnis.

Das mag zwar optisch nicht so schön eingerückt wirken, ist aber bei der Suche nach deklarationen umso praktischer.

[Christian18]

Ok, danke für den Tipp.

ich werde versuchen mich daran zu halten. Hast du eine Idee auf eine vorherige Frage?

MFG Christian18

[alzaimar]

Bezüglich der Formatierung würde ich einen der gängigen Codeformatter nehmen und mich danach nicht mehr drum kümmern.

Wenn du sauber programmieren möchtest, 'solltest' Du deinen Code auch kommentieren. Ansonsten finde ich dein Beispiel zu klein, um eine Aussage über sauberes Programmieren zu machen. Es reicht ja nicht, einige Formatierungskonventionen zu verfolgen (dafür gibts ja Formatierer). Viel wichtiger ist doch, wie Du deine Objekte definierst, ob Du immer ein geeignetes Pattern verwendest usw.

[sx2008]

Ich würde die Klasse TBackground nicht mit einem Formular in Verbindung bringen, sondern nur mit einem Canvas. Die Größe des Canvas wird dann über 2 Properties Width & Height mitgeteilt.
Vorteil: die Klasse arbeitet auch mit Printer.Canvas und TImage.Canvas zusammen.

Ausserdem sollten die beiden Farben auch als Property freigelegt werden; die Übergabe im Konstruktor ist so ja ziemlich restriktiv.

Ausserdem sollten deutlich mehr Background-Muster (Karos, Linien, Dreiecke, Sechsecke) programmiert werden, damit sich der Aufwand für eine Klasse überhaupt lohnt.

[Christian Seehase]

Moin Sebastian,

Zitat:

Zitat von Phoenix

Des öfteren Sucht man eine Deklaration um schnell dort hin zu gelangen. Ich würde dann z.B. immer nach "FColor:" suchen.

:gruebel:
Um eine Deklaration zu finden genügt es doch die entsprechende Variable mit STRG anzuklicken.

[Muetze1]

Zitat:

Zitat von Christian18

Muetze1, ich habe da mal ne Frage. Warum machst du alle Eigenschaften der Klasse TBackground über property öffentlcih und die Form nicht?

Wie könnte ich denn, wo mein Vorschlag die Form nicht in der Klasse hält? Die Klasse malt was auf eine Form - warum sollte ich die Klasse auf eine Instanz festlegen und somit beschränken? Somit übergebe ich einfach die Form in den beiden Routinen. Damit hat die TBackground Instanz immer die Farb- bzw. Hintergrundeinstellungen und kann dann auch x beliebige Formulare angewendet werden.

Den Constructor habe ich entfernt, weil er zu restriktiv ist, wie auch schon sx2008 geschrieben hat. Du kannst die Klasse anlegen und die farblichen Einstellungen jederzeit ändern bzw. anpassen.

@sx2008: Das mit dem Canvas war auch mein erster Ansatz, aber er nutzt die Color Eigenschaft der Form, was auch entsprechend Windows als Window Color mitgeteilt wird. Von daher konnte ich mich nicht auf ein TCanvas beschränken.