Zum Inhalt
wannacode

Code-Smells: lange Methode, große Klasse, Duplikate, Primitive statt Typen, lange if-Ketten

Kurz gesagt: Ein Code-Smell ist ein Zeichen, dass die nächste Änderung einen Fehler hervorbringt: eine lange Methode, eine große Klasse, Duplikate, Primitive statt Typen und eine lange Kette von ifs.

Problem

Die Kasse sagt 90 €, die Rechnung 95 €

Die Summe einer Bestellung wird an zwei Stellen berechnet: an der Kasse und in der Rechnung. Vor einem Monat wurde der VIP-Rabatt auf 10 % erhöht. Der Entwickler hat die Kasse korrigiert:

def checkout_total(subtotal, vip):
    discount = subtotal * 0.10 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery




def invoice_total(subtotal, vip):
    discount = subtotal * 0.05 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery
print(f"Kasse: {checkout_total(100, True):.2f} €")
print(f"Rechnung: {invoice_total(100, True):.2f} €")
Ausgabe
Kasse: 90.00 €
Rechnung: 95.00 €

Die Kundin zahlt 90 €, und auf der Rechnung steht 95 €. Die Buchhaltung findet seit einem Monat bei jeder VIP-Bestellung eine Differenz von 5 €. Keine Zeile hier ist für sich „falsch“: invoice_total tut genau das, wofür sie geschrieben wurde. Das Problem ist, dass eine Regel an zwei Stellen lebt.

Sieh es dir an

Wie zwei Kopien auseinanderdriften

def checkout_total(subtotal, vip):
    discount = subtotal * 0.10 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery




def invoice_total(subtotal, vip):
    discount = subtotal * 0.05 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery




print(f"Kasse: {checkout_total(100, True):.2f} €")
print(f"Rechnung: {invoice_total(100, True):.2f} €")
Schritt für Schritt, in Worten
  1. invoice_total entstand als Copy-Paste von checkout_total: dieselben drei Zeilen, beide mit 5 % VIP-Rabatt.
  2. Der VIP-Rabatt wird 10 %. Der Fehler wurde an der Kasse gemeldet, also kommt die Korrektur in checkout_total, Zeile 2.
  3. Niemand erinnert sich, dass die Rechnung eine Kopie ist. Zeile 8 behält 0.05: Die zwei Kopien sagen jetzt Verschiedenes.
  4. Eine Bestellung über 100 € einer VIP-Kundin läuft durch die Kasse: 100 − 10 + 0 = 90 €.
  5. Dieselbe Bestellung geht in die Rechnung: 100 − 5 + 0 = 95 €.
  6. Die Kundin zahlt 90 €, und auf der Rechnung steht 95 €. Eine Regel an zwei Stellen: Du korrigierst die eine, die andere bleibt zurück.

Am Tag des Copy-Paste waren beide Funktionen richtig. Der Fehler entstand später, mit einer Korrektur, die ehrlich gemeint war, aber nur an einer Stelle gemacht wurde. Code, der heute richtig ist, aber bei der nächsten Änderung einen Fehler auslöst, hat einen Code-Smell. Der Begriff stammt von Kent Beck und Martin Fowler: Ein Smell ist kein Fehler, sondern ein Zeichen, dass in seiner Nähe bald etwas schiefgeht.

In dieser Lektion geht es um die fünf häufigsten Smells und darum, wie du sie bemerkst, bevor sie dich 5 € pro Bestellung kosten.

Die Idee

Duplikate: eine Regel, eine Stelle

Die Regel für die Summe wird eine Funktion, und die Kasse und die Rechnung rufen beide sie auf:

def order_total(subtotal, vip):
    discount = subtotal * 0.10 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery
print(f"Kasse: {order_total(100, True):.2f} €")
print(f"Rechnung: {order_total(100, True):.2f} €")
Ausgabe
Kasse: 90.00 €
Rechnung: 90.00 €

Jetzt kannst du nicht die eine korrigieren und die andere vergessen: Es gibt keine andere.

Woran du es riechst. Du korrigierst einen Fehler und fragst dich: „Wurde das irgendwo kopiert?“ Du suchst nach einer auffälligen Zahl oder einem Ausdruck (0.05) und findest ihn an zwei Stellen. Zwei Funktionen mit fast gleichen Namen (checkout…, invoice…) und fast gleichen Rümpfen.

Vorsicht. Nicht alles, was gleich aussieht, ist ein Duplikat. Kostenlose Lieferung ab 50 € und Rückgabe innerhalb von 50 Tagen sind zwei Regeln mit einer Zahl. Fasst du sie zu einer Konstante zusammen, ändert die Änderung der einen die andere. Ein Duplikat ist eine Regel an zwei Stellen. Viele Teams warten auf die dritte Kopie, bevor sie den gemeinsamen Teil herausziehen: Zwei können Zufall sein, drei selten.

Sieh es dir an

Lange Methode

Die Funktion nimmt eine Bestellung und macht alles:

def place_order(email, subtotal, city):
    # Prüfung
    if "@" not in email:
        print("Ungültige E-Mail: " + email)
        return
    if subtotal <= 0:
        print("Leerer Warenkorb")
        return
    # Summe
    delivery = 3.90 if city == "Sofia" else 5.90
    if subtotal >= 50:
        delivery = 0
    total = subtotal + delivery
    # Benachrichtigung
    text = f"Deine Bestellung über {total:.2f} € ist angenommen."
    print("An " + email + ": " + text)
    # Log
    print(f"[log] Bestellung {email} {total:.2f}")
place_order("ana@example.com", 34, "Plovdiv")
place_order("ana.example.com", 34, "Plovdiv")
Ausgabe
An ana@example.com: Deine Bestellung über 39.90 € ist angenommen.
[log] Bestellung ana@example.com 39.90
Ungültige E-Mail: ana.example.com

Woran du es riechst: Du scrollst, um das Ende zu sehen; Kommentare dienen als Überschriften für Teile („Prüfung“, „Summe“); du kannst nicht sagen, was sie tut, ohne „und“ zu sagen; eine Variable, die oben deklariert wird, wird erst ganz unten benutzt. Jeder Überschriften-Kommentar ist der Name einer Funktion, die darauf wartet, zu entstehen (siehe die Lektion Kommentare, die lügen).

Jeder Teil kommt in eine eigene Funktion, benannt nach seinem Kommentar:

def problem_with(email, subtotal):
    if "@" not in email:
        return "Ungültige E-Mail: " + email
    if subtotal <= 0:
        return "Leerer Warenkorb"
    return None




def delivery_fee(subtotal, city):
    if subtotal >= 50:
        return 0
    return 3.90 if city == "Sofia" else 5.90




def notify_customer(email, total):
    text = f"Deine Bestellung über {total:.2f} € ist angenommen."
    print("An " + email + ": " + text)




def log(email, total):
    print(f"[log] Bestellung {email} {total:.2f}")
def place_order(email, subtotal, city):
    problem = problem_with(email, subtotal)
    if problem is not None:
        print(problem)
        return
    total = subtotal + delivery_fee(subtotal, city)
    notify_customer(email, total)
    log(email, total)
place_order("ana@example.com", 34, "Plovdiv")
place_order("ana.example.com", 34, "Plovdiv")
Ausgabe
An ana@example.com: Deine Bestellung über 39.90 € ist angenommen.
[log] Bestellung ana@example.com 39.90
Ungültige E-Mail: ana.example.com

Die Ausgabe ist dieselbe, Buchstabe für Buchstabe. Die Hauptfunktion liest sich jetzt wie eine Liste von Schritten, und die Liefergebühr lässt sich einzeln testen. Dieser Schritt heißt Extract Method, und du machst ihn in kleinen Schritten in der Lektion zum Refactoring.

Sieh es dir an

Große Klasse

Die Bestellung hat alles gesammelt, was jemals jemand gebraucht hat:

@dataclass
class Order:
    customer_name: str = ""
    customer_email: str = ""
    customer_phone: str = ""
    delivery_city: str = ""
    delivery_street: str = ""
    delivery_postcode: str = ""
    card_number: str = ""
    card_expiry: str = ""
    subtotal: float = 0
    discount: float = 0
    sms_sent: bool = False
    email_sent: bool = False
order = Order()
order.delivery_city = "Varna"
print("Lieferung nach " + order.delivery_city)
Ausgabe
Lieferung nach Varna

Woran du es riechst: Gruppen von Feldern mit gemeinsamem Präfix (customer…, delivery…, card…); Methoden, die nur eine Gruppe benutzen; ein Name wie OrderManager oder Utils, der nichts sagt; jede Aufgabe im Team fasst diese Datei an, und die Merge-Konflikte in Git sind immer darin.

Jede Gruppe mit gemeinsamem Präfix ist ein Typ, der auf einen Namen wartet:

@dataclass
class Customer:
    name: str
    email: str
    phone: str




@dataclass
class Address:
    city: str
    street: str
    postcode: str




@dataclass
class Card:
    number: str
    expiry: str




@dataclass
class Order:
    customer: Customer | None = None
    delivery: Address | None = None
    card: Card | None = None
    subtotal: float = 0
    discount: float = 0
    sms_sent: bool = False
    email_sent: bool = False
order = Order()
order.delivery = Address("Varna", "Hafenstraße 5", "9000")
print("Lieferung nach " + order.delivery.city)
Ausgabe
Lieferung nach Varna
@dataclass schreibt __init__, __repr__ und __eq__ aus den Feldern. Die Adresse ist jetzt ein Wert: Sie wird an einer Stelle übergeben, verglichen und geprüft. Address | None sagt, dass es auch gar keine Adresse geben kann.

Der Rest (smsSent, emailSent) ist die nächste Gruppe. Du musst nicht alles auf einmal korrigieren.

Probier es aus

Sag die Ausgabe voraus

Drei Produkte zu je 0,10 €. Was gibt das Programm aus?

total = 0
for _ in range(3):
    total += 0.10
print(total == 0.30)
print(total)
Antwort

0.10 hat keine exakte binäre Form, so wie 1/3 keine exakte dezimale Form hat. Ein double speichert die nächste binäre Zahl, und die kleinen Fehler summieren sich.

Sieh es dir an

Primitive statt Typen: Geld

Geld in einem double ist die häufigste Form des Smells Primitive Obsession: Ein Wert mit eigenen Regeln lebt in einem Typ, der diese Regeln nicht kennt. Bisher waren die Beträge im Kurs float, um es kurz zu halten. In echtem Code:

from decimal import Decimal


total = Decimal("0")
for _ in range(3):
    total += Decimal("0.10")
print(total == Decimal("0.30"))
print(total)
Ausgabe
True
0.30
Decimal aus dem Modul decimal speichert Dezimalstellen exakt. Es wird aus einem String erzeugt: Decimal(0.10) nimmt das schon falsche float und behält den Fehler.
from decimal import Decimal

total = Decimal("0")
for _ in range(3):
    total += Decimal()
print(total == Decimal("0.30"))
print(total)
Genauer, für Neugierige

Noch besser ist ein Typ Money, der den Betrag und die Währung zusammenhält und dich nicht aus Versehen Euro zu Dollar addieren lässt. Es ist dieselbe Idee wie beim Telefon auf der nächsten Karte.

Sieh es dir an

Primitive statt Typen: das Telefon

Eine Telefonnummer als String ist eine weitere Form desselben Smells. „0888 123 456“, „+359 888 123 456“ und „+359888123456“ sind eine Nummer, aber drei verschiedene Strings. Jede Stelle, die Telefonnummern vergleicht, muss selbst daran denken, wie man sie bereinigt. Der Typ Phone bereinigt und prüft einmal, wenn er erzeugt wird:

import re
from dataclasses import dataclass




@dataclass(frozen=True)
class Phone:
    value: str


    def __post_init__(self):
        digits = re.sub(r"[ ()-]", "", self.value)
        if digits.startswith("0"):
            digits = "+359" + digits[1:]
        if not re.fullmatch(r"\+359\d{9}", digits):
            raise ValueError("Ungültige Telefonnummer: " + self.value)
        object.__setattr__(self, "value", digits)
a = Phone("0888 123 456")
b = Phone("+359 888 123 456")
print(a.value)
print("gleiche Nummer" if a == b else "verschiedene Nummern")
try:
    Phone("0888 12")
except ValueError as e:
    print(e)
Ausgabe
+359888123456
gleiche Nummer
Ungültige Telefonnummer: 0888 12

Der reguläre Ausdruck [ ()-] entfernt Leerzeichen, Klammern und Bindestriche, und \+359\d{9} prüft, dass auf +359 genau neun Ziffern folgen.

Eine ungültige Telefonnummer kann nicht existieren: Der Konstruktor lässt sie nicht durch. Jedes Phone im Programm ist schon sauber.

frozen=True verbietet Änderungen nach der Erzeugung. Darum wird die saubere Nummer in __post_init__ mit object.__setattr__ gespeichert, an der einen Stelle, wo das vertretbar ist. == kommt von @dataclass.

Bring die Zeilen des Konstruktors in die Reihenfolge, damit aus "0888 123 456" die Nummer "+359888123456" wird:

  1. digits = "+359" + digits[1:]
  2. object.__setattr__(self, "value", digits)
  3. digits = re.sub(r"[ ()-]", "", self.value)
  4. if not re.fullmatch(r"\+359\d{9}", digits):
  5. raise ValueError("Ungültige Telefonnummer: " + self.value)
  6. if digits.startswith("0"):

Probier es aus

Finde den Fehler

Der Kunde hat SMS abbestellt, und der Shop schreibt ihm trotzdem bei jedem Paket:

opted_out = ["+359888123456"]
notify_shipped("0888 123 456", opted_out)
notify_shipped("0877 555 010", opted_out)
Ausgabe
SMS an 0888 123 456: Deine Bestellung ist unterwegs.
SMS an 0877 555 010: Deine Bestellung ist unterwegs.

In welcher Zeile steckt der Fehler?

Wähle die Zeile mit dem Fehler.

Sieh es dir an

Lange if-Kette über einen Typ

Der Preis und die Lieferzeit hängen von der Lieferart ab. Die Kette steht in beiden Funktionen:

def price(kind):
    if kind == "Kurier":
        return 5.90
    elif kind == "Filiale":
        return 3.90
    elif kind == "Packstation":
        return 2.90
    raise ValueError("Unbekannte Lieferart: " + kind)




def days(kind):
    if kind == "Kurier":
        return 1
    elif kind == "Filiale":
        return 2
    elif kind == "Packstation":
        return 3
    raise ValueError("Unbekannte Lieferart: " + kind)
for kind in ["Kurier", "Filiale", "Packstation"]:
    print(f"{kind}: {price(kind):.2f} €, Tage: {days(kind)}")
Ausgabe
Kurier: 5.90 €, Tage: 1
Filiale: 3.90 €, Tage: 2
Packstation: 2.90 €, Tage: 3

Woran du es riechst: dieselbe Kette über dasselbe Feld (kind, type, status) an mehreren Stellen; eine neue Art heißt, jede Kette zu finden; ein vergessener Zweig fällt erst auf, wenn ein Kunde ihn wählt.

Hier unterscheiden sich die Arten nur durch Zahlen. Dann wird die Kette zu einer Tabelle: Art → Daten:

@dataclass(frozen=True)
class Delivery:
    price: float
    days: int




DELIVERY = {
    "Kurier": Delivery(5.90, 1),
    "Filiale": Delivery(3.90, 2),
    "Packstation": Delivery(2.90, 3),
}
for kind in ["Kurier", "Filiale", "Packstation"]:
    d = DELIVERY[kind]
    print(f"{kind}: {d.price:.2f} €, Tage: {d.days}")
Ausgabe
Kurier: 5.90 €, Tage: 1
Filiale: 3.90 €, Tage: 2
Packstation: 2.90 €, Tage: 3

Eine neue Art ist eine neue Zeile in der Tabelle. Wenn sich die Arten im Verhalten unterscheiden und nicht nur in Zahlen, wird jede Art ihre eigene Klasse oder Funktion: Das ist Strategy. Wenn sich die Art mit der Zeit ändert (die Bestellung ist neu, dann bezahlt, dann versandt), ist es State. Wie aus einer Kette Schritt für Schritt Klassen werden (Replace Conditional with Polymorphism), steht in der Lektion zum Refactoring.

Die Idee

Ein Smell ist kein Fehler: wann du ihn lässt

Die fünf Smells in Kürze:

  • Duplikate: eine Regel an mehreren Stellen → eine Funktion oder Konstante;
  • lange Methode: du scrollst und liest Überschriften-Kommentare → Extract Method;
  • große Klasse: Gruppen von Feldern mit gemeinsamem Präfix → eine Klasse für jede Gruppe;
  • Primitive statt Typen: Geld in einem double, ein Telefon in einem String → ein Typ, der seine eigenen Regeln hält;
  • lange if-Kette über einen Typ: dieselbe Kette an vielen Stellen → eine Tabelle, Strategy oder State.

Ein Smell ist kein Fehler. Er sagt dir, was die nächste Änderung kosten wird. Du lässt ihn also, wenn:

  • der Code sich nicht ändert. Der Bericht, den seit drei Jahren niemand angefasst hat, kostet nichts, bis du ihn anfasst;
  • es keine Tests gibt. Ohne sie kann das Beseitigen des Smells etwas kaputt machen, das funktioniert hat. Zuerst Tests (die nächste Lektion), dann kleine Schritte;
  • zwei Dinge nur gleich aussehen. Zwei Regeln mit einer Zahl sind kein Duplikat;
  • der Code nur einmal gebraucht wird: ein Migrationsskript, das du morgen löschst.

Die Pfadfinderregel: Wenn du eine Datei sowieso anfasst, hinterlasse sie ein bisschen sauberer, als du sie vorgefunden hast. Schreib nicht „für alle Fälle“ ganze Module neu.

Übung

Rieche es und korrigiere es

Löse es in deinem eigenen Editor.

Der Bon. Die Funktion schickt einen Bon per SMS, E-Mail oder Viber:

def send_receipt(channel, contact, amount):
    if channel == "sms":
        text = f"Du hast {amount:.2f} € bezahlt. Danke!"
        print("SMS an " + contact.replace(" ", "") + ": " + text)
    elif channel == "email":
        text = f"Du hast {amount:.2f} € bezahlt. Danke!"
        print("E-Mail an " + contact.strip().lower() + ": " + text)
    elif channel == "viber":
        text = f"Du hast {amount:.2f} € bezahlt. Danke!"
        print("Viber an " + contact.replace(" ", "") + ": " + text)
send_receipt("sms", "0888 123 456", 24.5)
send_receipt("email", " Ana@Example.com ", 24.5)
send_receipt("viber", "0877 555 010", 9.9)
Ausgabe
SMS an 0888123456: Du hast 24.50 € bezahlt. Danke!
E-Mail an ana@example.com: Du hast 24.50 € bezahlt. Danke!
Viber an 0877555010: Du hast 9.90 € bezahlt. Danke!

Finde die Smells darin (mindestens drei) und benenne sie. Behebe sie dann einen nach dem anderen und führe das Programm nach jedem Schritt aus: Die Ausgabe muss gleich bleiben, Buchstabe für Buchstabe.

Weitere Aufgaben

  • Ein neuer Kanal. Füge Telegram hinzu, mit einem Kontakt wie bei der SMS. An wie vielen Stellen änderst du vor der Korrektur etwas, und an wie vielen danach?
  • Geld in Cent. Ändere den Betrag auf dem Bon auf ganze Cent (2450) und mach erst beim Ausgeben Euro daraus.
  • Die große Klasse. Mach Order von der Karte „Große Klasse“ fertig: Verschiebe smsSent und emailSent in einen eigenen Typ und überleg, wie du ihn nennst.

Test

Teste dich

1. Der Code hat einen Smell. Heißt das, dass er einen Fehler hat?
2. Kostenlose Lieferung gilt ab 50 €, und Rückgaben sind bis zu 50 Tage erlaubt. Solltest du daraus eine Konstante machen, um Duplikate zu vermeiden?
3. Summen werden in einem double addiert, und dreimal 0.10 ist nicht 0.30. Was hilft?
4. Dieselbe if-Kette über die Lieferart steht im Preis, in der Lieferzeit und in der Beschriftung. Eine neue Art kommt dazu. Was hilft?
5. Das Modul für den Jahresbericht: 300 Zeilen in einer Methode, seit drei Jahren hat sie niemand angefasst, keine Tests. Morgen musst du etwas anderes ausliefern. Was tust du?

Erstellt von Ivo Gergov im Rahmen der Kampagne von Evo Tech für zugängliche Programmierausbildung ·