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

[bernau]

Zitat von DeddyH:

Zitat von Coffeecoder:

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 von bernau:

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 von Klaus01:

Zitat von bernau:

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 von DeddyH:

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 von bernau:

Zitat von DeddyH:

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

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.









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.

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