Code Smells

[Coffeecoder]

Hallo DP-Mitglieder,

Ich habe ein kleinen Kurs über das Thema "Code Smells" für euch vorbereitet.
Das Ziel dieses Kurses ist das Verstehen was "Code Smells" sind. Denn jeder Programmierer hat (unbewusst) solche in seinem Code.

Theorie:

Das Wort "Code Smells" was übersetzt "(schlechter) Geruch" heißt,
stammt von Kent Beck und wurde durch das Buch Refactoring von Martin Fowler verbreitet.

Code Smells ist ein schlecht strukturierter Programmcode.
Der Code ist für den oder einen anderen Programmierer schwer zu verstehen.

Bei Verbesserungen oder Erweiterungen kommen neue Fehler dazu.
"Code Smells" wird erst erkannt bei einer Überarbeitung des Programmcodes.

Welche "Code Smells" existieren?

Es gibt eine Unmenge von schlecht-struktierter Programmcode-Beispiele.
Hier einige Beispiele die sich auf die objektorientierte Programmierung (Java, Delphi, ...) beziehen:

1. Code-Vermehrung: Der identische Code taucht im Programm an mehreren Stellen auf.
2. Langer Code in Methode: Der Programmcode in einer Funktion, Prozedur ist zu lang.
3. Lange Parameterliste: einige übergebene Parameter die unnötig sind.
4. Nichtaussagender Name: Aussagekräftige Name erfüllen das Verständnis des Codes oder Inhalt einer Variable.
5. Toter Code: Ein Programmcode welches nicht mehr gebraucht wird.

Quelle : http://de.wikipedia.org/wiki/Code_smells

Natürlich gibt es noch mehrere Arten und interessante Bücher die das Thema detaillierter
beschreiben wie z.B das Buch : "Clean Code - Refactoring, Patterns, Testen und Techniken für sauberen Code" von Robert C. Martin.

Beispiele/Tipps:

1. "Code-Smell"
   
   Delphi-Quellcode:

   procedure anpassenChart;
   begin
                   with Chart1 do
                   begin
                          Legend.visible := false;
                          View3D := false;
                          Title.Text.Clear;
                          Title.Text.Add('Chart');
                   end;
   
                   with Chart2 do
                   begin
                          Legend.visible := false;
                          View3D := false;
                          Title.Text.Clear;
                          Title.Text.Add('Chart');
                   end;
   
                   with Chart3 do
                   begin
                          Legend.visible := false;
                          View3D := false;
                          Title.Text.Clear;
                          Title.Text.Add('Chart');
                   end;
   end;

   Es ist nicht falsch, der Code funktionniert ja. Aber was ist schlecht daran?
   Ganz einfach: Es sollen jetzt doch Legende visible sein. Was macht man?
   Legend.visible := true setzen und Copy-Paste.
   Das ist anfällig für Fehler, einmal nicht aufgepasst und schon ein Legend.visible := false vergessen zu überschreiben.
   
   So macht man es besser:
   
   Delphi-Quellcode:

   procedure anpassenChart(ch : TChart);
   Begin
                   with ch do
                   begin
                         Legend.visible := false;
                         View3D := false;
                         Title.Text.Clear;
                         Title.Text.Add('Chart');
                   end;
   end;

   Aufruf:
   
   Delphi-Quellcode:

   ...
   var i : integer;
   begin
           for  i := 1 to 3 do anpassenChart('Chart'+IntToStr(i));
   end;

2. und
3. Beide Punkten sind verwandt, denn es geht ja um Methoden (Funktion, Prozedur).
   Hier zeige ich kein Beispiel, aber ein paar Tipps:
   1. Kurze Parameterliste, welche für eine einzige Aufgabe gebraucht werden.
   2. Es sollen nicht mehr als 20 Programmzeilen in einer Methode stehen.
   3. Eine Methode soll nur eine Aufgabe erfüllen. (eine Funktion die den Flächeninhalt eines Kreises berechnet, soll nicht noch den Umfang berechnen, dies sollte in einer separaten Funktion untergebracht werden.)
4. Ich komme sofort zu einem Beispiel:
   
   Schlechter Name:
   
   
   Delphi-Quellcode:

   var
        d : integer;
   begin
          ...
          if d  ... 
   end;

   Was will die Variable d uns sagen? Nicht viel!
   
   Besser:
   
   
   Delphi-Quellcode:

   ...
   var
        TageSeitErstellung : integer;
   
   begin
         ...
         if TageSeitErstellung ....
   
   end;

   Hier sagt die Variable TageSeitErstellung uns der Zweck dieser Variable aus. Man weiss sofort, der Inhalt ist die Anzahl Tage seit der Erstellung von ...
   
   Noch ein typisches Beispiel mit dem Typ boolean:
   
   
   Delphi-Quellcode:

   var
         Daten : boolean;

   Daten sagt wieder nichts aus. Man kann es so verstehen:
   1. DatenGeLoescht
   2. DatenVorhanden
   3. DatenGesendet
   4. DatenVerarbeitet
   5. DatenKorrupt
   6. ...
   
   
5. Toter Code ist :
   1. Ein auskommentierter Code
   2. Ein Codestück welches immer übersprungen wird. Hier ist die if-Anweisung überflüssig:
      
      Delphi-Quellcode:

      var
        N : int;
      
      begin
              N := 0;
              ...
              if N = 3 then
              begin
                   ....
              end;
              ...
      end;

Schlusswort:

Kennt man einige Code-Smells, so kann man diese vermeiden, damit der Programmcode nicht "schlecht riecht".

Ich hoffe ich könnte euch ein bisschen über dieses interessante Thema erzählen.
Bitte um konstruktive Kritik. :)

Danke bis Bald.

Mfg Coffeecoder

[MrOuzo]

Hallo Coffeecoder,

gefällt mir ganz gut, fällst bei deinem Beispiel dann über die eigenen Regeln.

ich denke

Delphi-Quellcode:

procedure anpassenChart(ch : TChart);
Begin
end;

hier ist ch auch nicht gerade ein aussagekräftiger Parameter, könnte auch ein Char oder so sein :-D

Gruß
MrOuzo

[Luckie]

Außerdem wird hier als Parameter eine Zeichenkette übergeben:

Delphi-Quellcode:

anpassenChart('Chart'+IntToStr(i));

laut Deklaration müsste es aber ein Objekt vom Typ TChart sein. Mir ist zwar klar, was du aussagen willst, aber trotzdem sollte es doch ein Beispiel sein, was zumindest compiliert.

[s.h.a.r.k]

Zitat:

Zitat von Luckie (Beitrag 1110268)

Außerdem wird hier als Parameter eine Zeichenkette übergeben:

Delphi-Quellcode:

anpassenChart('Chart'+IntToStr(i));

laut Deklaration müsste es aber ein Objekt vom Typ TChart sein. Mir ist zwar klar, was du aussagen willst, aber trotzdem sollte es doch ein Beispiel sein, was zumindest compiliert.

Ein einfaches FindComponent würde hier dann wohl schon reichen, sodass es klappt.

Zitat:

Zitat von MrOuzo (Beitrag 1110266)

Delphi-Quellcode:

procedure anpassenChart(ch : TChart);
Begin
end;

hier ist ch auch nicht gerade ein aussagekräftiger Parameter, könnte auch ein Char oder so sein :-D

Hier sollte man sich mal die VCL anschauen, die an sehr vielen stellen folgende Konvention nutzt:

Delphi-Quellcode:

procedure anpassenChart(AChart: TChart);
Begin
end;

Wobei das leider nicht immer der Fall ist -- ist mir neulich allein schon mal wieder bei der Format-Funktion aufgefallen.

Ansonsten muss ich sagen, dass das ein interessanter Einstieg sein kann :thumb:

[blackfin]

Dazu fällt mir folgende Aussage von einem Bekannten ein: "Wieso? Ich weiss doch, dass die Soundsystem-Klasse bei mir Peter heisst" :-D

[s.h.a.r.k]

Zitat:

Zitat von blackfin (Beitrag 1110287)

Dazu fällt mir folgende Aussage von einem Bekannten ein: "Wieso? Ich weiss doch, dass die Soundsystem-Klasse bei mir Peter heisst" :-D

Leider sieht man sowas bei viel zu vielen Programmierern. Liegt imho dann aber auch daran, dass diese noch nie im Team programmiert haben.

[brechi]

Wenn schon Verbesserung, dann bitte verwende keine With-Statements und rueck den Code richtig ein :)

[s.h.a.r.k]

Zitat:

Zitat von brechi (Beitrag 1110437)

Wenn schon Verbesserung, dann bitte verwende keine With-Statements und rueck den Code richtig ein :)

Was ist an with so böse?

[Luckie]

Man sieht nicht mehr auf was sich das with bezieht. Und mitunter kommt der Compiler schon mal durcheinander und dann sucht man sich dumm und dusselig.

[EWeiss]

Außerdem habe ich die Erfahrung gemacht das sich die nachfolgenden Variablen schlecht Debugen lassen.
Da Sie wegen Optimierung unterdrückt werden.

PS:
Und Zeilen sparrt man sich dadurch auch nicht wirklich.

gruss

[s.h.a.r.k]

Zitat:

Zitat von EWeiss (Beitrag 1110452)

Außerdem habe ich die Erfahrung gemacht das sich die nachfolgenden Variablen schlecht Debugen lassen.
Da Sie wegen Optimierung unterdrückt werden.

PS:
Und Zeilen sparrt man sich dadurch auch nicht wirklich.

gruss

Aber Schreibarbeit ;)

[EWeiss]

Zitat:

Zitat von s.h.a.r.k (Beitrag 1110453)

Zitat:

Zitat von EWeiss (Beitrag 1110452)

Außerdem habe ich die Erfahrung gemacht das sich die nachfolgenden Variablen schlecht Debugen lassen.
Da Sie wegen Optimierung unterdrückt werden.

PS:
Und Zeilen sparrt man sich dadurch auch nicht wirklich.

gruss

Aber Schreibarbeit ;)

Ahh jo Copy und Paste ... hehehehe :)

gruss

[Meflin]

Zitat:

Zitat von EWeiss (Beitrag 1110454)

Ahh jo Copy und Paste...

... ist sehr oft ein Zeichen dafür, dass ein Smell vorliegt :P

[Stevie]

Die "Notwendigkeit" für ein with ist wahrscheinlich Feature Envy, Indecent Exposure und/oder Message Chains

[FredlFesl]

Zitat:

Zitat von s.h.a.r.k (Beitrag 1110440)

Was ist an with so böse?

Zitat:

Zitat von Luckie (Beitrag 1110446)

...Und mitunter kommt der Compiler schon mal durcheinander...

Ganz sicher nicht. Der Compiler ist deterministisch, da der die Grammatik zumindest hier formal korrekt interpretiert.

Zitat:

Zitat von Stevie (Beitrag 1110457)

Die "Notwendigkeit" für ein with ist wahrscheinlich Feature Envy, Indecent Exposure und/oder Message Chains

So hab ich das noch nicht gesehen. Aber stimmt: Verwender des 'WITH' müssen nur ihren Code aufräumen (zur Not mit Class Helper), dann wird das 'WITH' meist überflüssig.

Mein Code riecht immer dann gut, wenn ich keine Kommentare benötige, da der Code flüssig zu lesen ist (wie eine Beschreibung) und einfach zu verstehen ist.
Weiterhin spüre ich, das ich in die richtige Richtung gehe, wenn die Klassen mit Scheuklappen durch die Gegend rennen, d.h. keine Abhängigkeiten zu anderen Klassen existieren. Dann kann ich mit den Klassen vieles anstellen, ohne mir einen abzubrechen.

Eine schöne Beschreibung für eine gut implementierte Methode ist von Dr.Bob: "Wenn Du das, was die Methode macht, beschreibst, und ohne das Wörtchen 'und' auskommst, hast Du etwas richtig gemacht".

[Deep-Sea]

Uhu, bricht hier wieder mal ein With-Glaubenskrieg aus? *schon mal das Popcorn hol* :stupid:

[DeddyH]

Delphi-Quellcode:

with Popcorn do
  Mampf;

:mrgreen:

[Coffeecoder]

Hey,

Ich danke für eure Feedbacks. Es ist mir klar, dass es schon bisschen her ist.

Zitat:

Zitat von MrOuzo (Beitrag 1110266)

Hallo Coffeecoder,

gefällt mir ganz gut, fällst bei deinem Beispiel dann über die eigenen Regeln.

ich denke

Delphi-Quellcode:

procedure anpassenChart(ch : TChart);
Begin
end;

hier ist ch auch nicht gerade ein aussagekräftiger Parameter, könnte auch ein Char oder so sein :-D

Gruß
MrOuzo

Das stimmt da falle ich selbst über die "Anti-Code-Smells"-Regeln. Besser soll heissen, wie bereits erwähnt:

Delphi-Quellcode:

procedure anpassenChart(pchart : TChart);
Begin
    ....
end;

Weiter gehts :)

Zitat:

Zitat von Luckie (Beitrag 1110268)

Außerdem wird hier als Parameter eine Zeichenkette übergeben:

Delphi-Quellcode:

anpassenChart('Chart'+IntToStr(i));

laut Deklaration müsste es aber ein Objekt vom Typ TChart sein. Mir ist zwar klar, was du aussagen willst, aber trotzdem sollte es doch ein Beispiel sein, was zumindest compiliert.

Da habe ich wieder nicht aufgepasst :?

Also ich bedanke mich für eure Hinweise und Vorschläge, ich werde das Tutorial wohl einmal überarbeiten müssen und gebe es als PDF auch frei.

Bis dahin, habt bisschen Geduld :)

[DeddyH]

Zitat:

Zitat von Coffeecoder (Beitrag 1145315)

Delphi-Quellcode:

procedure anpassenChart(pchart : TChart);
Begin
    ....
end;

Der Präfix P erweckt aber eher den Eindruck, es handle sich um einen Zeiger auf ein TChart. Wieso nicht einfach Chart, AChart oder meinetwegen TheChart?

[Coffeecoder]

Zitat:

Zitat von DeddyH (Beitrag 1145316)

Zitat:

Zitat von Coffeecoder (Beitrag 1145315)

Delphi-Quellcode:

procedure anpassenChart(pchart : TChart);
Begin
    ....
end;

Der Präfix P erweckt aber eher den Eindruck, es handle sich um einen Zeiger auf ein TChart. Wieso nicht einfach Chart, AChart oder meinetwegen TheChart?

Hmm ok. Ich habe gerade diesen Beitrag hier in der DP gefunden :)

[bernau]

Zitat:

Zitat von DeddyH (Beitrag 1145316)

Zitat:

Zitat von Coffeecoder (Beitrag 1145315)

Delphi-Quellcode:

procedure anpassenChart(pchart : TChart);
Begin
    ....
end;

Der Präfix P erweckt aber eher den Eindruck, es handle sich um einen Zeiger auf ein TChart. Wieso nicht einfach Chart, AChart oder meinetwegen TheChart?

Daran habe ich auch zuerst gedacht. Aber letztendlich ist es ein Zeiger auf ein Objekt vom Type TChart. Sieht trotzdem schrecklich aus ;-)

[Klaus01]

Zitat:

Zitat von bernau (Beitrag 1145325)

Daran habe ich auch zuerst gedacht. Aber letztendlich ist es ein Zeiger auf ein Objekt vom Type TChart. Sieht trotzdem schrecklich aus ;-)

Wenn man das dann zuende denkt- dan wäre ja jede Klasseninstanz auch schlichtweg "nur" ein Pointer.

Grüße
Klaus

[bernau]

Zitat:

Zitat von Klaus01 (Beitrag 1145327)

Zitat:

Zitat von bernau (Beitrag 1145325)

Daran habe ich auch zuerst gedacht. Aber letztendlich ist es ein Zeiger auf ein Objekt vom Type TChart. Sieht trotzdem schrecklich aus ;-)

Wenn man das dann zuende denkt- dan wäre ja jede Klasseninstanz auch schlichtweg "nur" ein Pointer.

Grüße
Klaus

Ja. Ist es.

[DeddyH]

Somit wäre dann aber ein PObject ein Pointer auf einen Pointer. Unschön, wenn nicht irgendwie beabsichtigt.

[bernau]

Zitat:

Zitat von DeddyH (Beitrag 1145333)

Somit wäre dann aber ein PObject ein Pointer auf einen Pointer. Unschön, wenn nicht irgendwie beabsichtigt.

So kann man es aus sehen. Das wäre sogar die richtige Interpretation. Daher auf keinen Fall ein p als Präfix für ein instanziertes Objekt. Verwirrt nur.

[Coffeecoder]

Zitat:

Zitat von bernau (Beitrag 1145335)

Zitat:

Zitat von DeddyH (Beitrag 1145333)

Somit wäre dann aber ein PObject ein Pointer auf einen Pointer. Unschön, wenn nicht irgendwie beabsichtigt.

So kann man es aus sehen. Das wäre sogar die richtige Interpretation. Daher auf keinen Fall ein p als Präfix für ein instanziertes Objekt. Verwirrt nur.

Ich ändere dies in der neuen Version zu "achart".

[himitsu]

Für mich sind Parameter mit Präfix einfach nur häßlich.

Typen mit T, Pointertypen mit P, Interfacetypen mit I, sowie Fields/Felder mit F, sind ja vollkommen OK,
aber Property- und Parameternamen auch noch verschandeln?



Aber wenn man schon soeinen Scheiß macht, dann sollte man das dann auch komplett durchzuiehen.

Globale Variablen mit G, Lokale mit L, die Parameter/Argumente mit A, Constanten mit C und für die Property oder die Klassenvariablen (Class Var) fällt uns bestimmt auch noch was ein.
Eventuell noch die nicht ganz "globalen" Variablen innerhalb der Implementation noch mit einem V davor :gruebel:

Und zum Schluß überall noch den kranken ungarischen Dreck davor.



Am Ende überlegt man sich das nameXCamelCase dann nochmal, findet es nun ebenfalls Scheiße und wechselt zu KA_WIE_DAS_HEISST. :angle2:









Wer unbedingt will, kann intern ja eine Umleitung einrichten.

Delphi-Quellcode:

procedure MeineProzedur(MeinParameter: MeinTyp);
var
  Argument_MeinParameter: MeinTyp absolute MeinParameter;  
begin
  if Argument_MeinParameter = 0815 then

end;

[Coffeecoder]

Es sind noch immer (nur) Konventionen ;)
[OT] In Java macht folgendes mit den Parameterübergabe:

Code:

...

public void setName(String name) {
       this.name = name;
}

Ohne Präfix :)
[/OT]

[himitsu]

[OT] Das mach ich in Delphi genauso. :stupid:

[bernau]

Du wirst lachen, aber lokale Variablen fangen bei mir tatsächlich mit (einem kleinen) L an. Wobei das nich ganz optimal ist, da man das "l" mit einer "1" (in Worten "Eins") verwechseln kann. Gibt auch einige Styleguides die das bemängeln. Da bei mir aber keine Variable mit einer Ziffer anfängt, kann ich das verschmerzen. Parameter/Argumente bekommen immer ein kleines "a". Hat den Vorteil, daß ich immer sehe, welche Variable lokal ist und welche von aussen kommt.

[Coffeecoder]

Zitat:

Zitat von bernau (Beitrag 1145343)

Du wirst lachen, aber lokale Variablen fangen bei mir tatsächlich mit (einem kleinen) L an. Wobei das nich ganz optimal ist, da man das "l" mit einer "1" (in Worten "Eins") verwechseln kann. Gibt auch einige Styleguides die das bemängeln. Da bei mir aber keine Variable mit einer Ziffer anfängt, kann ich das verschmerzen.

Ich dachte, du kannst keine Ziffer als Präfix vor einem Variablennamen schreiben :gruebel:
Dies ist doch nicht erlaubt resp der Compiler meckert doch : 1int;

[himitsu]

Das Einzige, was ich total bescheuert finde, das ist der string.

Ist erstmal eigenartig, daß diese (Basis)Typen kein T davor haben und dann das fette String.


Auf Arbeit schreibe ich es (da es überall so ist) klein, aber privat lieber groß. :oops:
Und warum schreibt man groß klein, aber das Kleine groß?
Blau

[DeddyH]

Wenn keine Verwechslungsgefahr (z.B. mit Properties) besteht, dann lasse ich Präfixe bei Argumenten auch am liebsten weg, ansonsten sollte der Weg mit dem vorangestellten A meistens funktionieren (es sei denn, es besteht wieder Verwechslungsgefahr). Ganz blöde wird es bei solchen Konstrukten (das with ist in diesem Zusammenhang natürlich Absicht):

Delphi-Quellcode:

constructor TMeineKomponente.Create(AOwner: TComponent; Name: string);
begin
  FDings := TDings.Create;
  with FDings do
    begin
      Name := Name;
    end;

[bernau]

Zitat:

Zitat von Coffeecoder (Beitrag 1145346)

Zitat:

Zitat von bernau (Beitrag 1145343)

Du wirst lachen, aber lokale Variablen fangen bei mir tatsächlich mit (einem kleinen) L an. Wobei das nich ganz optimal ist, da man das "l" mit einer "1" (in Worten "Eins") verwechseln kann. Gibt auch einige Styleguides die das bemängeln. Da bei mir aber keine Variable mit einer Ziffer anfängt, kann ich das verschmerzen.

Ich dachte, du kannst keine Ziffer als Präfix vor einem Variablennamen schreiben :gruebel:
Dies ist doch nicht erlaubt resp der Compiler meckert doch : 1int;

Deswegen fängt bei mir ja auch keine Variable mit einer Ziffer an ;-)

[himitsu]

Syntax für Alles ist [{Alpha}_][{AlphaNum}_]* aber eigentlich isses [{Letter}_][{Letter}{Digit}_]* und in ANSI-Delphis [a-zA-Z_][a-zA-Z0-9_]* bei Unitnamen und Componentennamen (TComponent.Name) [{Alpha}_][{AlphaNum}_.]* (früher waren keine Punkte erlaubt)
Aber dennoch besser keine mehrfach zusammenhängenden Punkte und keine Punkte am Ende.

Ab Delphi 2009 darf ich also auch Müssen, Muß oder 秘密 als Name verwenden. :angle:

Einige verdammen es, aber ich find's geil. :thumb: