Saubere Programmierung mit Delphi

[mkinzler]

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