Mein Programmierstyl

[Martin W]

Hi; Ich hoffe mal der Thread ist am richtigen Ort....

Delphi-Quellcode:

{*******************************************************}
{                                                       }
{        ANWENDUNGSNAME                                 }
{                                                       }
{        Copyright (c) 2006 Martin                      }
{                                                       }
{*******************************************************}

unit server_mainform_unit;

interface

...

Wenn ihr den COde seht... fallen euch Dinge auf die unsauber programmiert wurden? Oder gar falsch sind? Wie sind die Kommentare? Halte ich mich an "Standards"... Also ich will nicht das Programm korigiert haben, sondern Verbesserungsvorschläge zu meinem "Styl" haben..

So.. jetzt dürft ihr mich in Stücke reißen Aber bitte konstuktiv... wenn ich etwas grundverkehrt mache im Bezug auf der Verwendung von .net ---> Bitte sagt es mir!!!

Danke für alle Vorschläge !!!

Gruß
Martin W.

[edit=MrSpock]Code in den Anhang verschoben. Ist zu lang. Mfg, MrSpock[/edit]
[edit=Admin]Beitrag auf Wunsch des Autors editiert. Mfg, Daniel[/edit]
[edit=Admin] Mfg, Daniel[/edit]

[Bernhard Geyer]

1, Wieso benennst Du deine Resourcenstrings so verständlich (Str0031)?
Ich würde für 'Profile wurden gespeichert' cStrProfileSaved oder cProfileWurdeGespeichert nehmen.

2, Für Funktionskommentare gibt es schon diverse "Standards" um diese auch relative einfach in Hilfedateien zu überführen. Siehe z.B. Time2Help

3, Nicht alle Sprachabhängigen Texte wurden berücksichtigt. Siehe z.B. die Zeile
lOpenDialog.Title := 'Datei öffnen...'; . Schau dir im Rahmen von Mehrsprachigkeit auch gleich mal GNU GetText an. Geht evtl. auch für VCL.NET.

4, Ich würde die Logiklassen in eine eigene Unit packen. Damit erkennst Du auch einfacher ob du "unschönerweise" zu viele Methoden als Private definiert hast bzw. ob du noch einige Methoden besser als Public definierst.

5, Dateinamen wie FDS_Server_TCPPreferences.xml solltest Du als Konstanten definieren damit du nicht mal das Problem eines Schreibfehlers hast.

[Martin W]

Schon mal danke für deine Antwort! Ihr könnte mir ruhig weiterschreiben, aber ich muss jetzt erst mal arbeiten gehen...

Also nicht wundern wenn ihr vor 22 Uhr keine Antwort von mir bekommt !!!

Danke im Vorraus, ich werde nachher zu jedem einzelnen Posting von euch genau stellung nehmen Versprochen


Bis heute abend;

Gruß
Martin W

[Keldorn]

Hallo

ich würde außerdem noch das AType ändern.

Delphi-Quellcode:

type TSinnvollerNameTyp=(SNT_TCPSERVER,SNT_Profiles,SNT_Anwendungsklassen);

...

procedure Free_Classes(AType:TSinnvollerNameTyp); 

...

Aufruf:
  Free_Classes(SNT_TCPSERVER); 
  Free_Classes(SNT_Profiles); 
  Free_Classes(SNT_Anwendungsklassen);

Namensgebung mußt du halt noch anpassen, dass läßt sich dann aber wesentlich einfacher lesen als 1,2,3 und mit case kannst du genauso arbeiten und du sparst dir Kommentare wie

Delphi-Quellcode:

  // AType 
  // 1 :: TCP SERVER 
  // 2 :: Profiles

Mfg Frank

[BlackJack]

In Delphi ist es eigentlich eher nicht üblich, in Bezeichnern Unterstriche zur Trennung von Wörtern zu benutzen. Also sollte Free_Classes eher FreeClasses heissen oder auch statt Ein_sehr_langer_Methodenname das hier: EinSehrLangerMethodenname

[Robert Marquardt]

server_mainform_unit enthaelt unnoetige Namensbestandteile. Das es eine Unit ist weiss man doch schon.
"geg1" unnoetig abgekuerzt und nicht mit Grossbuchstaben vorne.
"Profile1" vs "Profile2" da stimmt doch was nicht. Nie gleiche Menuepunkte in veschiedenen Untermenues.
Besser alles in englisch. Ich hasse gemischtsprachliche Programme.
"Prepare_classes" besser "PrepareClasses". Man sollte sich strikt an den CamelCase halten und auch die Gross/Kleinschreibung durchziehen als ob Delphi casesensitiv waere. Es liest sich einfach leichter.
Desgleichen bei den Typen, also "Boolean", "Integer", "True", "False" usw. so wie die Delphi-Tooltips es vorgeben. Nur "string" ist da eine Ausnahme. Schluesselworte sollten klein geschrieben werden.
"Free_Classes" "besser "FreeClasses".
"{ Private-Deklarationen }" schmeiss diesen Mist raus. Da hast du doch genau schon das gemacht zu was der Kommentar dich auffordert.
"filetocheck: TFilename" besser "FileToCheck: TFileName" oder "FileToCheck: string". TFileName gibt es nur damit man in der IDE einen eigenen Property-Editor daran binden kann.
"TLog = class" in der JVCL bevorzugen wir immer eine explizite Ableitungsangabe. "TLog = class(TObject)".
"File_version", "server_mainform_form: Tserver_mainform_form", "Server_version" das sollte inzwischen klar sein.
"//********************** Ausführende Prozeduren ******************************//" nichtssagend und falsch und daher ueberfluessig.

{ TLog.AddMessageToLog(Sender: TMemo; TextMessage: string);
-----------------------------------------
Auszuführender Befehl:
- Fügt einem Memo eine Nachricht mit Uhrzeit hinzu}

Ebenso mit unnoetigen Bestandteilen versehen. Nur "Fügt einem Memo eine Nachricht mit Uhrzeit hinzu" enthaelt Information. Alles andere weglassen.
"timetostr(now)" besser "TimeToStr(Now)". Der CamelCase wurde zur besseren Lesbarkeit erfunden.

Sender.Lines.Add(timetostr(now) + ' : ' + TextMessage);
Solche Strings sollten immer mit Format zusammengebaut werden und der Formatstring sollte die Positionsangaben %1 ... enthalten.
Der Grund ist die Internationalisierung des Programms. Andere Sprachen koennen die Satzbestandteile unterschiedlich zusammenbauen.

"if server_mainform_form.IdTCPServer1.Active = True then" umstaendlich. Einfacher ist "if server_mainform_form.IdTCPServer1.Active then".
Prepare_classes ist gefaehrlich. Besser den Konstruktor der Form ueberschreiben und dort die Klassen anlegen. Entsprechend gehoert Free_Classes in den Destruktor.
"function Tserver_mainform_form.Import(AType: integer; AFilename: string): boolean;" ist falsch segmentiert. Das sind zwei unterschiedliche Aufgaben die in separate Methoden gehoeren.
"if FileExists(AFilename) = False then" besser "if not FileExists(AFileName) then".
"if RightStr(AFilename, 4) <> '.xml' then" besser "if ExtractFileExt(AFileName) <> '.xml' then", das zeigt besser an das es hier um Filenamen geht.
Ich hasse diese Vortests mit Exit.
"Free_Classes(1);" wenn man es nicht sowieso anders segmentieren muesste, dann muesste hier eine Enumeration fuer den Parameter verwendet werden. Der Name ist dann viel aussagekraeftiger und sicher gegen Fehleingaben. Eine 4 wuerde naemlich durch den Compiler gehen, aber nicht funktionieren.
"procedure Tserver_mainform_form.ExecuteDialog(AOnCanClose: TCloseQueryEvent; AType: TDialogType);" ueblicherweise will man doch Open- und Save-Dialoge als Komponenten auf der Form, damit sie sich merken koennen wo man schon mal war.
Wieder falsch segmentiert. Entweder zwei einzelne Methoden oder ausnutzen das TSaveDialog und TOpenDialog von TCommonDialog abstammen und alles nur einmal machen. Das hier ist klassisches Copy & Paste-Programmieren.
"FormCloseQuery" ist falsch. Das sollte alles nach "FormClose".
"'\FDS_Server_profiles.xml'" nie Stringliterale wiederholen sondern Konstanten benutzen. In diesem Fall "cServerProfile = 'FDS_Server_profiles.xml';". Entsprechend nie den Pfad explizit zusammenbauen, sondern in eine Methode konzentrieren. Das erlaubt es dann den Speicherort zentral zu aendern.
"FormShow" ist ein Versager. Die Methode wird oefters aufgerufen, aber "Prepare_Classes" ist nicht auf Mehrfachaufruf vorbereitet.
Besser keine unnoetigen begin end Bloecke benutzen. Zeilen die nicht da sind stoeren beim Lesen nicht.
"end; // case" Ich hasse diese Kommentare. Die Einrueckung sollte doch ausreichend sein.

[Robert Marquardt]

Eine Source hat nur zwei Zwecke: gelesen und veraendert zu werden.
Man sollte daher peinlichst darauf achten das man das immer im Auge behaelt.
Strikter CamelCase und strikter Namensaufbau unterstuetzen die Lesbarkeit.
Sorgfaeltige Segmentierung der Aufgaben in Methoden statt Copy & Paste erleichtert die Veraenderung.

[faux]

Hallo!

Was mir aufgefallen ist:

Delphi-Quellcode:

function Tserver_mainform_form.CheckFileExtension(filetocheck: TFilename; FileExtension: string): string;
begin
  // Dateinamen überprüfen oder korrigieren
  if RightStr(filetocheck, 4) <> FileExtension then
    begin
      Result := filetocheck + FileExtension;
    end
  else
    begin
      Result := filetocheck;
    end;
end;

Du hältst deine begins und ends nicht konstant am selben Platz.
Die sind direkt unter function, jedoch nicht direkt unter if bzw else. Wieso rückst du da ein?

Grüße
Faux

[Martin W]

@ Robert Marquardt ::

Zitat:

Sender.Lines.Add(timetostr(now) + ' : ' + TextMessage); Solche Strings sollten immer mit Format zusammengebaut werden und der Formatstring sollte die Positionsangaben %1 ... enthalten.
Der Grund ist die Internationalisierung des Programms. Andere Sprachen koennen die Satzbestandteile unterschiedlich zusammenbauen.

Zitat:

Prepare_classes ist gefaehrlich. Besser den Konstruktor der Form ueberschreiben und dort die Klassen anlegen. Entsprechend gehoert Free_Classes in den Destruktor.

Zitat:

"function Tserver_mainform_form.Import(AType: integer; AFilename: string): boolean;" ist falsch segmentiert. Das sind zwei unterschiedliche Aufgaben die in separate Methoden gehoeren.

Zitat:

...oder ausnutzen das TSaveDialog und TOpenDialog von TCommonDialog abstammen und alles nur einmal machen. Das hier ist klassisches Copy & Paste-Programmieren.

---> Was meinst du damit genau?

Zitat:

"if FileExists(AFilename) = False then" besser "if not FileExists(AFileName) then".
"if RightStr(AFilename, 4) <> '.xml' then" besser "if ExtractFileExt(AFileName) <> '.xml' then", das zeigt besser an das es hier um Filenamen geht.
Ich hasse diese Vortests mit Exit.

---> Wie soll man es sonst machen??? Also ohne Exit?? mit Endlos verschachtelten If Schleifen?

Zitat:

"FormShow" ist ein Versager. Die Methode wird oefters aufgerufen, aber "Prepare_Classes" ist nicht auf Mehrfachaufruf vorbereitet.

---> Sondern?


Gruß
Martin W.

[Robert Marquardt]

Delphi-Quellcode:

  RsEDateOutOfRange = '%0:s - Enter a date between "%1:s" and "%2:s"';
  RsEDateOutOfRange = 'Das Datum sollte zwischen "%1:s" und "%2:s liegen, aber enthält den Fehler %0:s"';
Format(RsEDateOutOfRange, ['Bla0', 'Bla1', 'Bla2']);

Das obige funktioniert mit beiden Formatstrings ohne das man sonst etwas an der Source aendern muss.

Prepare_classes ist gefaehrlich, denn es kann mehrfach aufgerufen werden. Dabei werden aber die Objekte jeweils neu angelegt, ohne das die bisherigen geloescht werden.
Ein neues Delphi-Projekt ist einfach eine Beispielableitung einer Komponente (TForm). In Prepare_classes werden Hilfsobjekte erstellt, die eigentlich doch ueber die gesamte Lebensdauer der Form vorhanden sein sollten. Deshalb sollte man den Konstruktor und Destruktor der Komponente/Form ueberschreiben und dort die Objekte erstellen bzw. loeschen.

Import hat zwei unterschiedliche Aufgaben. Einmal die Profiles und einmal die Preferences. Diese Aufgaben sollten von einander unabhaengig sein und deshalb auch in separaten Methoden implementiert sein.

Delphi-Quellcode:

procedure Tserver_mainform_form.ExecuteDialog(AOnCanClose: TCloseQueryEvent; AType: TDialogType);
var
  Dialog: TCommonDialog;
  Title: string;
begin
  case AType of
    dtOpen:
      begin
        Dialog := TOpenDialog.Create(server_mainform_form);
        Title := 'Datei öffnen...';
      end;
    dtSave:
      begin
        Dialog := TSaveDialog.Create(server_mainform_form);
        Title := 'Speichern unter...';
      end;
  end;

  try
    Dialog.DefaultExt := 'xml';
    Dialog.Filter := 'XML File / xml|*.xml';
    Dialog.Title := Title;
    Dialog.OnCanClose := AOnCanClose;
    Dialog.Options := [ofHideReadOnly,ofFileMustExist,ofEnableSizing];
    Dialog.Execute;
  finally
    Dialog.Free;
  end;
end;

---> Wie soll man es sonst machen??? Also ohne Exit?? mit Endlos verschachtelten If Schleifen?
Weniger Exit und mehr positive IFs ("if then DoSomething"). Man kann die Tests auch in eine lokale Funktion zusammenfassen.
Nach dem dritten "if not then Exit" beginnt es unlesbar zu werden.


Autor: Martin W
#9|BeitragVerfasst am: 19.04.2006, 01:40 Titel: Re: Mein Programmierstyl
Antworten mit Zitat
@ Robert Marquardt ::

Zitat:
Delphi-Quellcode: markieren
Sender.Lines.Add(timetostr(now) + ' : ' + TextMessage);

Solche Strings sollten immer mit Format zusammengebaut werden und der Formatstring sollte die Positionsangaben %1 ... enthalten.
Der Grund ist die Internationalisierung des Programms. Andere Sprachen koennen die Satzbestandteile unterschiedlich zusammenbauen.


Zitat:
Prepare_classes ist gefaehrlich. Besser den Konstruktor der Form ueberschreiben und dort die Klassen anlegen. Entsprechend gehoert Free_Classes in den Destruktor.


Zitat:
"function Tserver_mainform_form.Import(AType: integer; AFilename: string): boolean;" ist falsch segmentiert. Das sind zwei unterschiedliche Aufgaben die in separate Methoden gehoeren.


Zitat:
...oder ausnutzen das TSaveDialog und TOpenDialog von TCommonDialog abstammen und alles nur einmal machen. Das hier ist klassisches Copy & Paste-Programmieren.


---> Was meinst du damit genau?

Zitat:
"if FileExists(AFilename) = False then" besser "if not FileExists(AFileName) then".
"if RightStr(AFilename, 4) <> '.xml' then" besser "if ExtractFileExt(AFileName) <> '.xml' then", das zeigt besser an das es hier um Filenamen geht.
Ich hasse diese Vortests mit Exit.


---> Wie soll man es sonst machen??? Also ohne Exit?? mit Endlos verschachtelten If Schleifen?

---> Sondern?
Das was ich empfohlen habe.

Stell doch mal eine ueberarbeitete Version bereit.