Code Analyse von (semi) Profis

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

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

[p80286]

Zitat von Furtbichler:

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.

Wenn man Sourcen liest (Papier) ist das schon ein Unterscheid;

Zitat von Furtbichler:

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.

Wie üblich gibt es auch hier ein sowohl als auch. Wenn der Typ nicht direkt hinter dem Variablennamen steht, ist der Variablenname durchaus einfacher zu erfassen. wenn der Typ dann allerdings erst auf stelle 78 anfängt.....

Zitat von Furtbichler:

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.

Insbesonders wenn ich mich beim Debugging nicht wild durch die Sourcen wühlen will ist zumindestens ein Typhinweis nicht kontraproduktiv.

Achja Kommentare, Darin sollte stehen warum man etwas mach, das wie sollte eigentlich aus dem Quelltext hervor gehen.

Gruß
K-H

[Furtbichler]

Zitat von p80286:

Wenn man Sourcen liest (Papier) ist das schon ein Unterschied

Man schreibt Code nicht für diesen sehr unwahrscheinlichen Fall, sondern für den Normalfall: Die Programmierer sitzen vor dem Bildschirm um müssen fremden Code verändern. Es gilt aber auch: Ich muss mich der Situation anpassen: Entwickle ich in PHP und weiß, das alle Kollegen nur Notepad benutzen, sieht mein Code so aus, das man ihn auch ohne IDE schnell versteht.

Zitat von Furtbichler:

Wie üblich gibt es auch hier ein sowohl als auch. Wenn der Typ nicht direkt hinter dem Variablennamen steht, ist der Variablenname durchaus einfacher zu erfassen.

Zum Verständnis ist der Datentyp unerheblich. Ich lese Code und keine Deklarationen, um zu verstehen, was passiert. Aber wenn ich den Typen denn mal wissen will, scrolle ich ganz bestimmt nicht zur Deklaration (ob sie nun hübsch tabellarisch ist oder nicht): Ich halte den Cursor drauf, das ist irgendwie einfacher. Das geht zwar im Debugmodus nicht, aber i.A. debugge ich eh erst, wenn ich den Code verstanden habe. Und wenn, dann stringe ich eben per Strg+Click zur Deklaration und wieder zurück.

Nicht umsonst bietet C# hier ein 'var' anstatt der expliziten Typbezeichnug an: Weil es egal ist. Der Typ der Variablen ist zu 99% eh aus dem Code ersichtlich. Diese wird fast immer initialisiert, also ist klar, was das für ein Typ ist.

Delphi-Quellcode:

a := 1.0;  // Ganz klar
foo := TFoobar.Create; // Auch

Zitat:

Insbesonders wenn ich mich beim Debugging nicht wild durch die Sourcen wühlen will ist zumindestens ein Typhinweis nicht kontraproduktiv.

Nun ja. Schau einfach, was die Variable enthält, das ist in 99,9% der Fälle eh das, was dich interessiert. Die ungarische Notation greift ja nicht bei Klassen, sondern nur bei einfachen Typen. Und 'Strg-Click' ist eigentlich kein 'wild durch die Sourcen wühlen'.

Und wann ist es wichtig, ob das nun ein Byte, Word, DWord, Unsigned schießmichtot oder wasauchimmer ist? Eigentlich nie, außer z.B. bei hardwarenaher Programmierung. Und dann soll man das auch so machen. Die Regel lautet: Verwende klare Bezeichner. Und wenn es wichtig ist, das der Status ein Byte ist (und genau ein Byte!), dann schreibt man das eben: StatusByte. Aber als Dogma bzw. allgemeine Regel? 'ubStatus'... Nee.

Zitat:

Achja Kommentare, Darin sollte stehen warum man etwas mach, das wie sollte eigentlich aus dem Quelltext hervor gehen.

Bei mir würde dann stehen: "To make money" (Das ist der Grund, warum ich den Code schreibe).

Hast Du ein paar Beispiele parat, wann beim Debuggen der Typ einer Variablen wichtig ist? Wie würde ein Kommentar bei dir aussehen, bei dem Du beschreibst, warum Du die Lösung so und nicht anders implementiert hast? Enthält der Kommentar einen Mehrwert oder ist das eher Prosa, die durchaus interessant sein kann, aber eher ins Tagebuch gehört?

[bennySB]

Ich merke schon ich habe hier eine kleine Glaubensfrage gestartet xD

[BUG]

Zitat von Furtbichler:

Wie würde ein Kommentar bei dir aussehen, bei dem Du beschreibst, warum Du die Lösung so und nicht anders implementiert hast?

1. Hinweise auf Entwurfsmuster
2. Quellen und wichtige Eigenschaften der verwendeten Algorithmen
3. Dokumentation von Sachen, deren Gründe sonst schlecht aus dem Code ersichtlich wären.
   (zB. Maßnahmen gegen das Auftreten von False Sharing)
4. Gründe für Lösungen, wo man sich gegen das intuitive/kanonische Vorgehen entschieden hat.
   (zB. Dokumentation für Workarounds bei Fehlern in verwendeten Bibliotheken)

Natürlich kann einiges davon durch Verweise auf weitere Dokumentation ersetzt werden (z.B. im internen Wiki zum Projekt).

[Furtbichler]

Hi, bis auf #1 stimmte ich dem zu. Punkt 1 ist für Lehrveranstaltungen sinnvoll, aber nicht in produktivem Code.

Aber jedem das Seine.

[dataspider]

Wenn man bedenkt, das es der erste Code des TS ist, der dafür eine erstaunlich hohe Qualität hat...
... das er sich bereits jetzt in dieser Phase um sauberen Code Gedanken macht - Alle Achtung!

Wenn ich dann aber Hinweise zu ungarischer Notation lese... dann mach ich mir schon Sorgen.

Ich musste mal eine FIBU zerlegen (Namens Moses mit Oracle - DB und Unique als Sprache).
Dort hatten alle Tabellen- und Feldnamen ein U am Anfang.

Wenn du dann Feldnamen suchst und du hast eine List wie
UAnzBez
UStrasse
UOrt
U...

dann merkst du erst, wie bescheuert das ist und wie schwer das zu lesen ist.
Es dauert einfach viel länger!
Irgendwann war mal eine Sendung, da wurden Worte nur einen Bruchteil einer Sekunde eingeblendet.
Auch recht komplexe Worte hat man erkannt, obwohl man gar nicht so schnell lesen kann.

Und so kann man auch Code sehr schnell erfassen, wenn die Namen von Methoden, Variablen etc. eben lesbar sind.

Aus meiner Sicht ist das mit das Wichtigste und man kann nicht früh genug damit anfangen, lesbaren Code zu schreiben.
Man kann sich damit sooo viel Arbeit ersparen!

Frank

[Furtbichler]

Da hast Du die Diskussion sehr schnell wieder in die Spur gebracht. Worum ging es? Um einen Anfänger, der sich Gedanken um lesbaren Code macht und *abliefert*.