Code Analyse von (semi) Profis

[Dalai]

Auch wenn ich weder Profi noch Semiprofi bin, gebe ich meinen Senf dazu. Ich kann nahpets in allen Punkten zustimmen (bis auf das "begin" hinter "then", aber das ist Geschmackssache). Ich möchte noch einen Punkt ergänzen: Einrückung/Ausrichtung von Variablentypdefinitionen. Damit lässt sich IMO schneller eine optische Erfassung von Variablenname und dessen Typ erreichen. Konkret würde ich also die Definitionen so formatieren:

Code:

var
  UserDirPageCAE        : TInputDirWizardPage;
  DOSBoxConf            : TStringList;
  GPSDirCheckBox        : TRadioButton;
  SplashImage           : TBitmapImage;
  SplashForm            : TForm;
  SetupMajor            : dword;
  SetupMinor            : dword;
  SavedMajor            : dword;
  SavedMinor            : dword;

  CAE2000Path                              : string;
  DOSBoxCAE2000Path                        : string;
  DOSBoxCAE2000UninstallName               : string;
  DOSBoxCAE2000UninstallNameRemovedQuotes  : string;
  DOSBoxCAE2000UninstallPath               : string;
  DOSBoxCAE2000UninstallPathRemovedQuotes  : string;
  DOSBoxCAE2000UninstallString             : string;
  DOSBoxCAE2000UninstallStringRemovedQuotes: string;
  GPSPath                                  : string;
  GetCAE2000Path                           : string;
  SplashFileName                           : string;
  UserFolderCAE                            : string;
  tmp                                      : string;

OK, das sieht jetzt bei den langen Variablennamen ein bisschen Scheiße aus, aber bei deutlich kürzeren Namen erhöht es (für mich) die Lesbarkeit und fördert die schnelle Erfassung sowohl des Namens als auch dessen Typ.

Komischerweise entfernt das Forum aus obigem Code die wiederholten Leerzeichen, wenn er in DELPHI-Tags steht , daher in CODE-Tags.

MfG Dalai

[nahpets]

Genau so mache ich das bei den Variabeldeklarationen auch.
So kann man auch mal schnell nach Typ sortieren, halt Sotierung ab der Position vom :,
wobei ich den Typ (meist) auch durch die ungarische Notation bereits am Beginn des Variablennamens erkennen kann. Dadurch brauche ich beim Lesen von Quelltext nicht mehr bei der Deklaration nachschauen, von welchem Typ eine Variabel ist. Das sähe dann in etwa so aus:

Code:

var
  UserDirPageCAE                              : TInputDirWizardPage;
  slDOSBoxConf                                : TStringList;
  rbGPSDirCheckBox                            : TRadioButton;
  bmpSplashImage                              : TBitmapImage;
  fmSplashForm                                : TForm;
  dwSetupMajor                                : dword;
  dwSetupMinor                                : dword;
  dwSavedMajor                                : dword;
  dwSavedMinor                                : dword; 
  sCAE2000Path                                : string;
  sDOSBoxCAE2000Path                          : string;
  sDOSBoxCAE2000UninstallName                 : string;
  sDOSBoxCAE2000UninstallNameRemovedQuotes    : string;
  sDOSBoxCAE2000UninstallPath                 : string;
  sDOSBoxCAE2000UninstallPathRemovedQuotes    : string;
  sDOSBoxCAE2000UninstallString               : string;
  sDOSBoxCAE2000UninstallStringRemovedQuotes  : string;
  sGPSPath                                    : string;
  sGetCAE2000Path                             : string;
  sSplashFileName                             : string;
  sUserFolderCAE                              : string;
  stmp                                        : string;

[bennySB]

Wow hätte nicht gedacht das dich welche so auf meinen Code stürzen xD
Freut mich ein bißchen was an Anregung bekommen zu haben und besonders das Thema Einrückungen werde ich mal angreifen und noch so das ein oder andere.
Auf jedenfall schonmal danke an euch.

[DeddyH]

Da es noch niemand erwähnt hat: man tut nicht mit true oder false vergleichen tun

[bennySB]

Oh Ok warum denn das nicht?
Denn binär-vergleiche sind doch eigentlich das sicherste was es gibt. Bei 0 oder 1 kann nichts vertauscht werden.

[DeddyH]

Typische Anfängerfehler
Vergleichen Sie niemals mit Boolean-Konstanten

[dataspider]

Hi,

es ist wirklich lesbar und damit schon recht ordentlich.
Worauf ich noch achten würde:

function IsSetupNewer: boolean; Vom Name her würde ich nur erwarten, dass geprüft und ein Boolean zurückgeliefert wird.
Du ermittelst in dieser Funktion aber Werte für übergreifende Variablen,
wertest Result aus und reagierst mit Meldungen bzw. startest UnInstall...
Das Alles ist beim Lesen des Codes nicht zu vermuten, man muss erst in die Funktion, um zu erkennen,
das z.B. ResultAktuelleAppFound ermittelt wird etc.
Eine Funktion sollte in aller Regel nur EINE Aufgabe erfüllen, und das muss am Name erkennbar sein.

Diese riesigen Header mit viel Kommentaren halte ich für überflüssig. Wenn du diese nicht mehr brauchst, in dein Code lesbar.
Das ist IMHO eher anzustreben als sich in ewigen Kommentaren zu verlieren, die man bei Änderung des Codes nicht mit korrigiert und uns irgendwann in die Irre leiten.

Allerdings musste ich auch erst "Clean Code" lesen, um umzudenken.

Frank


Hier würde ich mehrere kleinere Routinen draus machen.

[kwhk]

False = 0
True = <> 0 (Alles was nicht NULL ist)
Wenn ich also prüfe if xx = True , dann kann xx alles <> 0 sein - wenn das passt OK, dann aber gleich auf if xx <> 0 testen.

[Luckie]

Eventuell für dich interessant: http://michael-puff.de/Programmierun...leanCode.shtml

[Furtbichler]

10 Programmierer, 20 Meinungen. Alle gleichwertig. Hier ist meine:

Wenn Du wirklich blutiger Anfänger bist, ziehe ich meinen Hut. Das ist -für dein Level- richtig gut lesbarer Code.

Aaaber

Ich würde aus DOSBOXCAE2000Uninstall erst einmal ein Record machen. Dann ist der Name zwar schön lang, aber mir sagt er nix. Wenn er Dir bzw. demjenigen, der das Programm kennt, etwas sagt: Gut. Wenn nicht: Umbenennen.

Zitat:

Einrückung/Ausrichtung von Variablentypdefinitionen...

Eigentlich ist es zweitrangig, von welchem Typ die Variablen sind. Wenn ich es wissen möchte, gehe ich mit dem Cursor rauf.

Zitat:

...und fördert die schnelle Erfassung sowohl des Namens als auch dessen Typ.

Wenn der Variablenname sehr weit von der Deklaration entfernt ist? Das Auge muss ja mühsam versuchen, beim nach-rechts-blicken in der gleichen Zeile zu bleiben.

Variablen- und Typbezeichnungen tabellarisch anzuordnen sieht vielleicht 'ordentlich' aus, aber gut bzw. lesbar bzw. 'schnell erfassbar' ist das nicht.

Zitat:

wobei ich den Typ (meist) auch durch die ungarische Notation bereits am Beginn des Variablennamens erkennen kann. Dadurch brauche ich beim Lesen von Quelltext nicht mehr bei der Deklaration nachschauen, von welchem Typ eine Variabel ist.

Siehe oben: Die ungarische Notation war bei C sinnvoll, da implizite Typkonvertierungen schnell zu Programmfehlern führte und als die IDE in etwa den Funktionsumfang von Notepad hatte, aber heutzutage braucht man das nicht mehr. Und da Code imho lesbar wie ein (englisches) Buch sein sollte, ist Code wie
if (hmHuman.dwMode=mdHungry) then nicht wirklich besser lesbar, als z.B. if (Human.Mode=Hungry) , wobei ich hier eher schreiben würde if Human.IsHungry . Zum Verständnis der Funktionsweise einer Methode ist es unerheblich, von welchem Typ eine Variable ist. Nebenbei: Was passiert, wenn sich der Typ doch mal ändert? Dann muss ich über Refactoring den Namen überall ändern. Was für eine überflüssige Arbeit.

Jede zweite Zeile würde ich auch nicht kommentieren. Wenn man Programmzeilen kommentieren muss, dann packt man sie in eine eigene Prozedur, der man einen aussagekräftigen Namen gibt.

Delphi-Quellcode:

 // Neuere Version ist installiert
   Result := (SetupMajor > SavedMajor)
          or ((SetupMajor = SavedMajor) and (SetupMinor >= SavedMinor));
   // Gleiche Version ist bereits installiert
   ResultAktuelleAppFound := (SetupMajor = SavedMajor)
                          or ((SetupMajor = SavedMajor) and (SetupMinor = SavedMinor));

   // Ermitteln und Umformen des Deinstallationspfades
   RegQueryStringValue(HKLM,'
 {#UNINSTKEY}',

Ich würde das so schreiben

Delphi-Quellcode:

Result := NewerVersionIsInstalled();
ResultAktuelleAppFound = SameVersionIsInstalled();
DOSBoxCAE2000Uninstall.ExtractPath();

Das ist für mich lesbarer und kommt ganz ohne Kommentare aus. Wenn ich wissen will, was z.B. 'ExtractPath' macht, navigiere ich eben rein. Aber um die eigentliche Funktion 'IsSetupNew' zu verstehen, ist es unerheblich, ob der Pfad in der Registry steht oder nicht. Nebenbei: Heißt es nun 'Deinstallation' oder 'Uninstall'?

Dann (eigentlich als Erstes) würde ich -wie Frank schon gesagt hat- die Routinen in kleinere Routinen aufteilen. Jede Einzelroutine macht genau ein Ding und sie heißt dann auch so. Und -schwupps- sind Kommentare im Code überflüssig. Delphi unterstützt dich hier durch die 'Refactoring-Extract Method' Funktion.

Prozeduren/Funktionen würde ich auch nicht kommentieren, denn so ein Kommentar wird selten mitgepflegt. Und überflüssig ist er auch:

Delphi-Quellcode:

// Entschlüsselung der Versionsnummer
function DecodeVersion(const dwMajor, dwMinor : dword) : string;
...
// Versionsabgleich der vorhandenen Installation zur neuen Installation
function IsSetupNewer : boolean;

Wozu dient dieser Kommentar? Wo ist der Mehrwert ggü dem Funktionsnamen? Wieso kommentiere ich in deutsch, verwende jedoch englische Bezeichner? Dann kommentiere ich nicht, sondern übersetze für deutsche Leser. Und wenn ich eh nur deutsche Leser/Programmierer im Fokus habe, wieso verwende ich dann englische Bezeichner? Entscheide dich für eine Sprache. Und zwar für eine, die Du gut beherrscht. Wenn Du meinst, kommentieren zu müssen, formatiere den Code um, bis er lesbar ist.

Ein generelles Wort zu Kommentaren: Kommentare sind fast immer böse, sinnlos, müllen den Code zu und sind zudem auch noch falsch.

Böse sind sie, weil sie einem etwas erklären wollen, was i.a. sowieso nicht (mehr) stimmt.
Sinnlos sind sie, weil sie Dinge erklären, die eh da stehen (Code). Wenn der Code so kompliziert ist, das man ihn kommentieren muss, dann schreibt man ihn eben leserlich, sodaß man ihn nicht kommentieren muss.
Falsch sind Kommentare deshalb, weil sie spätestens bei der 3.Änderung nicht mehr nachgepflegt werden.

Man benötigt keine Kommentare, um Code zu erklären. Ja gut. Fast keine. Also: Man benötigt fast nie Kommentare.
Ausnahmen: Quellenangaben, Gesetzesbestimmungen (Copyright, aber auch Implementierungsdetails, MWST-Rechnung etc.), Anmerkungen zum verwendeten Verfahren u.ä.

Delphi-Quellcode:

// Sorts the list using Quicksort
Procedure TMyThing.Sort();

Ist also doch besser als (es gilt ja auch: Implementierungsdetails verbergen, d.h. nicht in den Bezeichner packen)

Delphi-Quellcode:

//
Procedure TMyThing.QuickSort();

Abschließend noch ein Wort zu 'Tabs', 'Leerzeichen', 'Einrückung' generell: Da jeder einen Code-Beautifier in seiner IDE oder seinem Portfolio (=externes Tool) haben sollte, ist jegliche Diskussion darüber imho überflüssig. Wenn mir der Code nicht gefällt, drücke ich auf Ctrl+D (bei mir formatiert das den Code) und schon bekomme ich keinen Augenkrebs mehr. Ich muss mich mit dem Autor nicht streiten, ob nun Tabs oder Leerzeichen besser sind, oder das 'begin' eingerückt wird oder nicht oder auf einer eigenen Zeile steht oder nicht und ob Typdeklarationen untereinanderstehen. Nein. CTRL+D löst alle dies Probleme.

Nur eines ist wichtig (zig mal gesagt): Wenn Du im Team arbeitest, sollte alle die gleichen Einstellungen im Beautifier vornehmen. Code wird aber nicht ständig formatiert, sondern nur in 'Beautifier-Sessions'. Wieso? Wenn ich Codeänderungen in meiner Versionsverwaltung prüfe (Welche Codezeilen wurden denn in der Klasse XY verändert), geht der Bugfix unter, wenn gleichzeitig der Code mal wieder aufgehübscht wurde. Denn dann zeigt mir mein CVS alle Zeilen als 'verändert' an.

Also: Code wird hübsch, indem er lesbar, d.h. verständlich wird. Lesbar und Verständlich wird er, wenn man die für das Verständnis unerheblichen Teile versteckt, z.B. durch Verlagerung in kleine private Methoden mit einem verständlichen Namen.
Kommentare sollten überflüssig sein, bzw. knete deinen Code so lange, bis sie überflüssig werden, denn die Dinger altern schneller als eine Banane.

Einrückung etc. ist wie Make-Up. Kann man mal eben raufklatschen, aber 'hübscher' wird der Code dadurch auch nicht.