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 decimal CheckoutTotal(decimal subtotal, bool vip)
{
    decimal discount = vip ? subtotal * 0.10m : 0;
    decimal delivery = subtotal >= 50 ? 0 : 4.90m;
    return subtotal - discount + delivery;
}

static decimal InvoiceTotal(decimal subtotal, bool vip)
{
    decimal discount = vip ? subtotal * 0.05m : 0;
    decimal delivery = subtotal >= 50 ? 0 : 4.90m;
    return subtotal - discount + delivery;
}

Console.WriteLine($"Kasse: {CheckoutTotal(100, true):F2} €");
Console.WriteLine($"Rechnung: {InvoiceTotal(100, true):F2} €");

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 decimal CheckoutTotal(decimal subtotal, bool vip)
{
    decimal discount = vip ? subtotal * 0.10m : 0;
    decimal delivery = subtotal >= 50 ? 0 : 4.90m;
    return subtotal - discount + delivery;
}

static decimal InvoiceTotal(decimal subtotal, bool vip)
{
    decimal discount = vip ? subtotal * 0.05m : 0;
    decimal delivery = subtotal >= 50 ? 0 : 4.90m;
    return subtotal - discount + delivery;
}

Console.WriteLine($"Kasse: {CheckoutTotal(100, true):F2} €");
Console.WriteLine($"Rechnung: {InvoiceTotal(100, true):F2} €");

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 3.
3. Niemand erinnert sich, dass die Rechnung eine Kopie ist. Zeile 10 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 decimal OrderTotal(decimal subtotal, bool vip)
{
    decimal discount = vip ? subtotal * 0.10m : 0;
    decimal delivery = subtotal >= 50 ? 0 : 4.90m;
    return subtotal - discount + delivery;
}

Console.WriteLine($"Kasse: {OrderTotal(100, true):F2} €");
Console.WriteLine($"Rechnung: {OrderTotal(100, true):F2} €");

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, decimal subtotal, string city)
{
    // Prüfung
    if (!email.Contains("@"))
    {
        Console.WriteLine("Ungültige E-Mail: " + email);
        return;
    }
    if (subtotal <= 0)
    {
        Console.WriteLine("Leerer Warenkorb");
        return;
    }
    // Summe
    decimal delivery = city == "Sofia" ? 3.90m : 5.90m;
    if (subtotal >= 50)
    {
        delivery = 0;
    }
    decimal total = subtotal + delivery;
    // Benachrichtigung
    string text = $"Deine Bestellung über {total:F2} € ist angenommen.";
    Console.WriteLine("An " + email + ": " + text);
    // Log
    Console.WriteLine($"[log] Bestellung {email} {total:F2}");
}

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, decimal subtotal)
{
    if (!email.Contains("@"))
    {
        return "Ungültige E-Mail: " + email;
    }
    if (subtotal <= 0)
    {
        return "Leerer Warenkorb";
    }
    return null;
}

static decimal DeliveryFee(decimal subtotal, string city)
{
    if (subtotal >= 50)
    {
        return 0;
    }
    return city == "Sofia" ? 3.90m : 5.90m;
}

static void NotifyCustomer(string email, decimal total)
{
    string text = $"Deine Bestellung über {total:F2} € ist angenommen.";
    Console.WriteLine("An " + email + ": " + text);
}

static void Log(string email, decimal total)
{
    Console.WriteLine($"[log] Bestellung {email} {total:F2}");
}

static void PlaceOrder(string email, decimal subtotal, string city)
{
    string problem = ProblemWith(email, subtotal);
    if (problem != null)
    {
        Console.WriteLine(problem);
        return;
    }
    decimal 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
{
    public string CustomerName { get; set; }
    public string CustomerEmail { get; set; }
    public string CustomerPhone { get; set; }
    public string DeliveryCity { get; set; }
    public string DeliveryStreet { get; set; }
    public string DeliveryPostcode { get; set; }
    public string CardNumber { get; set; }
    public string CardExpiry { get; set; }
    public decimal Subtotal { get; set; }
    public decimal Discount { get; set; }
    public bool SmsSent { get; set; }
    public bool EmailSent { get; set; }
}

var order = new Order();
order.DeliveryCity = "Varna";
Console.WriteLine("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
{
    public Customer Customer { get; set; }
    public Address Delivery { get; set; }
    public Card Card { get; set; }
    public decimal Subtotal { get; set; }
    public decimal Discount { get; set; }
    public bool SmsSent { get; set; }
    public bool EmailSent { get; set; }
}

var order = new Order();
order.Delivery = new Address("Varna", "Hafenstraße 5", "9000");
Console.WriteLine("Lieferung nach " + order.Delivery.City);

Ausgabe

Lieferung nach Varna

Ein record mit Positionsparametern schreibt die Properties, den Konstruktor und den Vergleich nach Wert. 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;
}
Console.WriteLine(total == 0.30);
Console.WriteLine(total);

Antwort

True / 0.3

True / 0.30

False / 0.30000000000000004

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

decimal total = 0;
for (int i = 0; i < 3; i++)
{
    total += 0.10m;
}
Console.WriteLine(total == 0.30m);
Console.WriteLine(total);

Ausgabe

True
0.30

C# hat decimal, das Dezimalstellen exakt speichert, und deshalb war Geld in diesem Kurs von Anfang an decimal. Das Literal hat ein m: 0.10m. Ohne das m ist die Zahl ein double und lässt sich ohne explizite Umwandlung nicht zu einem decimal addieren.

decimal total = 0;
for (int i = 0; i < 3; i++)
{
    total += 0.10;
}
Console.WriteLine(total == 0.30m);
Console.WriteLine(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:

using System.Text.RegularExpressions;

record Phone
{
    public string Value { get; }

    public Phone(string raw)
    {
        string digits = Regex.Replace(raw, "[ ()-]", "");
        if (digits.StartsWith("0"))
        {
            digits = "+359" + digits.Substring(1);
        }
        if (!Regex.IsMatch(digits, @"^\+359\d{9}$"))
        {
            throw new ArgumentException("Ungültige Telefonnummer: " + raw);
        }
        Value = digits;
    }
}

var a = new Phone("0888 123 456");
var b = new Phone("+359 888 123 456");
Console.WriteLine(a.Value);
Console.WriteLine(a == b ? "gleiche Nummer" : "verschiedene Nummern");
try
{
    new Phone("0888 12");
}
catch (ArgumentException e)
{
    Console.WriteLine(e.Message);
}

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. Regex kommt aus System.Text.RegularExpressions; wenn du es noch nicht benutzt hast, reicht es hier zu wissen, was es prüft.

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

Ein record vergleicht nach Wert: a == b ist wahr, wenn Value gleich ist.

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

1. {↑↓
2. Value = digits;↑↓
3. {↑↓
4. digits = "+359" + digits.Substring(1);↑↓
5. }↑↓
6. throw new ArgumentException("Ungültige Telefonnummer: " + raw);↑↓
7. if (!Regex.IsMatch(digits, @"^\+359\d{9}$"))↑↓
8. }↑↓
9. if (digits.StartsWith("0"))↑↓
10. string digits = Regex.Replace(raw, "[ ()-]", "");↑↓

Prüfen

Probier es aus

Finde den Fehler

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

var optedOut = new List<string> { "+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{3 if (optedOut.Contains(phone))4 {5 return;6 }7 Console.WriteLine("SMS an " + phone + ": Deine Bestellung ist unterwegs.");8}

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 decimal Price(string kind)
{
    if (kind == "Kurier")
    {
        return 5.90m;
    }
    else if (kind == "Filiale")
    {
        return 3.90m;
    }
    else if (kind == "Packstation")
    {
        return 2.90m;
    }
    throw new ArgumentException("Unbekannte Lieferart: " + kind);
}

static int Days(string kind)
{
    if (kind == "Kurier")
    {
        return 1;
    }
    else if (kind == "Filiale")
    {
        return 2;
    }
    else if (kind == "Packstation")
    {
        return 3;
    }
    throw new ArgumentException("Unbekannte Lieferart: " + kind);
}

foreach (string kind in new[] { "Kurier", "Filiale", "Packstation" })
{
    Console.WriteLine($"{kind}: {Price(kind):F2} €, 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:

record Delivery(decimal Price, int Days);

static readonly Dictionary<string, Delivery> Deliveries = new()
{
    ["Kurier"] = new Delivery(5.90m, 1),
    ["Filiale"] = new Delivery(3.90m, 2),
    ["Packstation"] = new Delivery(2.90m, 3),
};

foreach (string kind in new[] { "Kurier", "Filiale", "Packstation" })
{
    Delivery d = Deliveries[kind];
    Console.WriteLine($"{kind}: {d.Price:F2} €, 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: 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, decimal amount)
{
    if (channel == "sms")
    {
        string text = $"Du hast {amount:F2} € bezahlt. Danke!";
        Console.WriteLine("SMS an " + contact.Replace(" ", "") + ": " + text);
    }
    else if (channel == "email")
    {
        string text = $"Du hast {amount:F2} € bezahlt. Danke!";
        Console.WriteLine("E-Mail an " + contact.Trim().ToLowerInvariant() + ": " + text);
    }
    else if (channel == "viber")
    {
        string text = $"Du hast {amount:F2} € bezahlt. Danke!";
        Console.WriteLine("Viber an " + contact.Replace(" ", "") + ": " + text);
    }
}

SendReceipt("sms", "0888 123 456", 24.5m);
SendReceipt("email", " Ana@Example.com ", 24.5m);
SendReceipt("viber", "0877 555 010", 9.9m);

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 rundendecimal statt double

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