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

Code in:JavaC#PythonJavaScript

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:

static double checkoutTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.10 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

static double invoiceTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.05 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

System.out.printf("Kasse: %.2f €%n", checkoutTotal(100, true));
System.out.printf("Rechnung: %.2f €%n", invoiceTotal(100, true));

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“: invoiceTotal 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

static double checkoutTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.10 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

static double invoiceTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.05 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

System.out.printf("Kasse: %.2f €%n", checkoutTotal(100, true));
System.out.printf("Rechnung: %.2f €%n", invoiceTotal(100, true));

Schritt für Schritt, in Worten

1. invoiceTotal entstand als Copy-Paste von checkoutTotal: 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 checkoutTotal, 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:

static double orderTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.10 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

System.out.printf("Kasse: %.2f €%n", orderTotal(100, true));
System.out.printf("Rechnung: %.2f €%n", orderTotal(100, true));

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 Methode nimmt eine Bestellung und macht alles:

static void placeOrder(String email, double subtotal, String city) {
    // Prüfung
    if (!email.contains("@")) {
        System.out.println("Ungültige E-Mail: " + email);
        return;
    }
    if (subtotal <= 0) {
        System.out.println("Leerer Warenkorb");
        return;
    }
    // Summe
    double delivery = city.equals("Sofia") ? 3.90 : 5.90;
    if (subtotal >= 50) {
        delivery = 0;
    }
    double total = subtotal + delivery;
    // Benachrichtigung
    String text = String.format("Deine Bestellung über %.2f € ist angenommen.", total);
    System.out.println("An " + email + ": " + text);
    // Log
    System.out.printf("[log] Bestellung %s %.2f%n", email, total);
}

placeOrder("ana@example.com", 34, "Plovdiv");
placeOrder("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:

static String problemWith(String email, double subtotal) {
    if (!email.contains("@")) {
        return "Ungültige E-Mail: " + email;
    }
    if (subtotal <= 0) {
        return "Leerer Warenkorb";
    }
    return null;
}

static double deliveryFee(double subtotal, String city) {
    if (subtotal >= 50) {
        return 0;
    }
    return city.equals("Sofia") ? 3.90 : 5.90;
}

static void notifyCustomer(String email, double total) {
    String text = String.format("Deine Bestellung über %.2f € ist angenommen.", total);
    System.out.println("An " + email + ": " + text);
}

static void log(String email, double total) {
    System.out.printf("[log] Bestellung %s %.2f%n", email, total);
}

static void placeOrder(String email, double subtotal, String city) {
    String problem = problemWith(email, subtotal);
    if (problem != null) {
        System.out.println(problem);
        return;
    }
    double total = subtotal + deliveryFee(subtotal, city);
    notifyCustomer(email, total);
    log(email, total);
}

placeOrder("ana@example.com", 34, "Plovdiv");
placeOrder("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 Hauptmethode 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:

class Order {
    String customerName;
    String customerEmail;
    String customerPhone;
    String deliveryCity;
    String deliveryStreet;
    String deliveryPostcode;
    String cardNumber;
    String cardExpiry;
    double subtotal;
    double discount;
    boolean smsSent;
    boolean emailSent;
}

Order order = new Order();
order.deliveryCity = "Varna";
System.out.println("Lieferung nach " + order.deliveryCity);

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:

record Customer(String name, String email, String phone) {}

record Address(String city, String street, String postcode) {}

record Card(String number, String expiry) {}

class Order {
    Customer customer;
    Address delivery;
    Card card;
    double subtotal;
    double discount;
    boolean smsSent;
    boolean emailSent;
}

Order order = new Order();
order.delivery = new Address("Varna", "Hafenstraße 5", "9000");
System.out.println("Lieferung nach " + order.delivery.city());

Ausgabe

Lieferung nach Varna

Ein record schreibt die Felder, den Konstruktor und die Zugriffsmethoden. Die Adresse ist jetzt ein Wert: Sie wird an einer Stelle übergeben, verglichen und geprüft.

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?

double total = 0;
for (int i = 0; i < 3; i++) {
    total += 0.10;
}
System.out.println(total == 0.30);
System.out.println(total);

Antwort

true / 0.3

false / 0.30000000000000004

true / 0.30

Prüfen

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 double, um es kurz zu halten. In echtem Code:

BigDecimal total = BigDecimal.ZERO;
for (int i = 0; i < 3; i++) {
    total = total.add(new BigDecimal("0.10"));
}
System.out.println(total.compareTo(new BigDecimal("0.30")) == 0);
System.out.println(total);

Ausgabe

true
0.30

BigDecimal speichert Dezimalstellen exakt. Es wird aus einem String erzeugt: new BigDecimal(0.10) nimmt das schon falsche double und behält den Fehler. Verglichen wird mit compareTo, weil equals 0.30 und 0.3 als verschieden behandelt.

BigDecimal total = BigDecimal.ZERO;
for (int i = 0; i < 3; i++) {
    total = total.add(new BigDecimal());
}
System.out.println(total.compareTo(new BigDecimal("0.30")) == 0);
System.out.println(total);

Prüfen

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:

record Phone(String value) {
    Phone {
        String digits = value.replaceAll("[ ()-]", "");
        if (digits.startsWith("0")) {
            digits = "+359" + digits.substring(1);
        }
        if (!digits.matches("\\+359\\d{9}")) {
            throw new IllegalArgumentException("Ungültige Telefonnummer: " + value);
        }
        value = digits;
    }
}

Phone a = new Phone("0888 123 456");
Phone b = new Phone("+359 888 123 456");
System.out.println(a.value());
System.out.println(a.equals(b) ? "gleiche Nummer" : "verschiedene Nummern");
try {
    new Phone("0888 12");
} catch (IllegalArgumentException e) {
    System.out.println(e.getMessage());
}

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.

Der kompakte Konstruktor eines record bekommt value, bevor es gespeichert wird, und kann es ändern. equals kommt vom record und vergleicht die sauberen Werte.

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

1. }↑↓
2. value = digits;↑↓
3. digits = "+359" + digits.substring(1);↑↓
4. throw new IllegalArgumentException("Ungültige Telefonnummer: " + value);↑↓
5. if (digits.startsWith("0")) {↑↓
6. String digits = value.replaceAll("[ ()-]", "");↑↓
7. if (!digits.matches("\\+359\\d{9}")) {↑↓
8. }↑↓

Prüfen

Probier es aus

Finde den Fehler

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

List<String> optedOut = List.of("+359888123456");
notifyShipped("0888 123 456", optedOut);
notifyShipped("0877 555 010", optedOut);

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.

1static void notifyShipped(String phone, List<String> optedOut) {2 if (optedOut.contains(phone)) {3 return;4 }5 System.out.println("SMS an " + phone + ": Deine Bestellung ist unterwegs.");6}

Prüfen

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:

static double price(String kind) {
    if (kind.equals("Kurier")) {
        return 5.90;
    } else if (kind.equals("Filiale")) {
        return 3.90;
    } else if (kind.equals("Packstation")) {
        return 2.90;
    }
    throw new IllegalArgumentException("Unbekannte Lieferart: " + kind);
}

static int days(String kind) {
    if (kind.equals("Kurier")) {
        return 1;
    } else if (kind.equals("Filiale")) {
        return 2;
    } else if (kind.equals("Packstation")) {
        return 3;
    }
    throw new IllegalArgumentException("Unbekannte Lieferart: " + kind);
}

for (String kind : List.of("Kurier", "Filiale", "Packstation")) {
    System.out.printf("%s: %.2f €, Tage: %d%n", kind, price(kind), 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:

record Delivery(double price, int days) {}

static final Map<String, Delivery> DELIVERY = Map.of(
    "Kurier", new Delivery(5.90, 1),
    "Filiale", new Delivery(3.90, 2),
    "Packstation", new Delivery(2.90, 3));

for (String kind : List.of("Kurier", "Filiale", "Packstation")) {
    Delivery d = DELIVERY.get(kind);
    System.out.printf("%s: %.2f €, Tage: %d%n", kind, d.price(), 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: 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:

static void sendReceipt(String channel, String contact, double amount) {
    if (channel.equals("sms")) {
        String text = String.format("Du hast %.2f € bezahlt. Danke!", amount);
        System.out.println("SMS an " + contact.replace(" ", "") + ": " + text);
    } else if (channel.equals("email")) {
        String text = String.format("Du hast %.2f € bezahlt. Danke!", amount);
        System.out.println("E-Mail an " + contact.trim().toLowerCase() + ": " + text);
    } else if (channel.equals("viber")) {
        String text = String.format("Du hast %.2f € bezahlt. Danke!", amount);
        System.out.println("Viber an " + contact.replace(" ", "") + ": " + text);
    }
}

sendReceipt("sms", "0888 123 456", 24.5);
sendReceipt("email", " Ana@Example.com ", 24.5);
sendReceipt("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?

Ja, ein Smell ist ein Fehler unter anderem NamenNein. Der Code kann heute funktionieren; der Smell sagt, dass die nächste Änderung riskant istJa, wenn der Compiler eine Warnung ausgibt

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?

Nein. Das sind zwei verschiedene Regeln, die zufällig dieselbe Zahl habenJa, eine Zahl soll an einer Stelle lebenJa, aber nenn sie FIFTY

3. Summen werden in einem double addiert, und dreimal 0.10 ist nicht 0.30. Was hilft?

Ein größerer Typ: float statt doubleJedes Mal beim Ausgeben rundenBigDecimal, aus einem String erzeugt

4. Dieselbe if-Kette über die Lieferart steht im Preis, in der Lieferzeit und in der Beschriftung. Eine neue Art kommt dazu. Was hilft?

Ein Kommentar über jeder Kette, der die Arten aufzähltEine Tabelle Art → Daten, oder eine Klasse pro Art, wenn sich das Verhalten unterscheidetEin weiteres else if an allen drei Stellen

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?

Ich teile sie heute in Funktionen aufIch lösche sieIch lasse sie. Ich bringe sie in Ordnung, wenn ich sie ändern muss, und schreibe vorher Tests dafür

Prüfen

Lektion von vorn beginnen

Verwandte Lektionen

• Baut auf: State · Namen im Code · Funktionen: eine Aufgabe, wenige Parameter, keine versteckten Seiteneffekte · Kommentare, die lügen, und Kommentare, die helfen
• Geht weiter in: Tests als Sicherheitsnetz · Refactoring in kleinen Schritten · SOLID ohne Dogma · Übung: eine chaotische Klasse, Schritt für Schritt aufgeräumt
• Aufgaben zum Thema: Sauberer Code und Refactoring