emrevarol.com

Vorlesungsmaterialien

Best Practices beim Programmieren und Effektives Code Review

A2SV - University of Rwanda

Präsentation ansehen

A2SV Code Review Checkliste

Der Goldstandard. Zehn Kategorien zur Bewertung jedes Pull Requests, den Sie prüfen.

1 Klarheit und Lesbarkeit
Fragen
  • Sind Funktions- und Variablennamen klar?
  • Kann ich den Code verstehen, ohne raten zu müssen?
  • Ist die Absicht offensichtlich?
  • Ist die Logik leicht nachvollziehbar?
Warnsignale
Namen wie x, tmp, data, calc, process; tief verschachtelte Logik; große unklare Funktionen
2 Funktions- und Designqualität
Fragen
  • Erfüllt jede Funktion genau eine klare Aufgabe?
  • Ist die Geschäftslogik von Seiteneffekten getrennt?
  • Ist das Design leicht testbar?
  • Macht eine Funktion zu viel?
Warnsignale
Validierung + Speichern + E-Mail + Logging in einer Funktion; Print-Ausgaben innerhalb von Logikfunktionen; fest kodiertes Verhalten über die gesamte Datei verteilt
3 Korrektheit
Fragen
  • Löst der Code das Problem tatsächlich korrekt?
  • Sind Annahmen explizit?
  • Werden alle Fälle behandelt?
Warnsignale
Fehlende Eingabevalidierung; Dictionary-Key-Annahmen; magische Werte; stille Fehler
4 Grenzfälle
Fragen
  • Was passiert bei leerer Eingabe?
  • Was passiert bei ungültiger Eingabe?
  • Was passiert bei fehlenden Schlüsseln?
  • Was passiert bei Null- oder negativen Werten?
Warnsignale
Nur der Normalfall berücksichtigt; keine defensiven Prüfungen, wo sie nötig sind
5 Tests
Fragen
  • Sind Tests enthalten?
  • Prüfen die Tests exakte Ergebnisse?
  • Werden Grenzfälle abgedeckt?
  • Würden diese Tests Regressionen erkennen?
Warnsignale
Nur ein Test; schwache Assertions wie > 0; keine Testfälle für ungültige Eingaben; keine Tests für geänderte Logik
6 Duplizierung
Fragen
  • Wird Logik wiederholt?
  • Kann gemeinsame Logik extrahiert werden?
  • Sind Geschäftsregeln an mehreren Stellen dupliziert?
Warnsignale
Wiederholte Formeln; wiederholte Bedingungsblöcke; wiederholte fest kodierte Werte
7 Fehlerbehandlung
Fragen
  • Werden Fehler klar behandelt?
  • Scheitert der Code auf verständliche Weise?
  • Sind ungültige Zustände sichtbar?
Warnsignale
Rückgabe von None ohne Erklärung; verschluckte Exceptions; versteckte Fehlermodi
8 Wartbarkeit
Fragen
  • Wird dies später leicht erweiterbar sein?
  • Kann ein anderer Ingenieur dies sicher ändern?
  • Macht dies die Codebasis sauberer oder unordentlicher?
Warnsignale
Fest kodierte Regeln; übergroßer PR; eng gekoppelte Logik
9 Repository-Qualität
Fragen
  • Ist die Repo-Struktur klar?
  • Ist die README nützlich?
  • Kann ein neuer Ingenieur das Projekt einfach starten?
  • Sind Abhängigkeiten sichtbar?
Warnsignale
Keine Einrichtungsanweisungen; schwache README; verwirrendes Layout; fehlendes Tests-Verzeichnis
10 PR-Qualität
Fragen
  • Ist der PR fokussiert?
  • Ist der Titel aussagekräftig?
  • Ist die Beschreibung hilfreich?
  • Ist die Änderungsgröße angemessen?
Warnsignale
Vage Titel; keine Erklärung; gemischte zusammenhanglose Änderungen; riesige PRs

Review-Kommentar-Vorlagen

Professionelle Formulierungen für häufige Review-Szenarien. Verwenden Sie diese als Ausgangspunkt.

Benennung
„Könnten wir diese Funktion umbenennen, um ihren Zweck besser widerzuspiegeln?“
„Dieser Variablenname wirkt zu allgemein. Können wir ihn aussagekräftiger machen?“
Logik
„Könnten wir diese Bedingung vereinfachen, um die Lesbarkeit zu verbessern?“
„Wäre es sinnvoll, diese Logik in eine Hilfsfunktion auszulagern?“
Tests
„Können wir einen Test für den Fall einer leeren Eingabe hinzufügen?“
„Könnten wir hier den exakt erwarteten Wert prüfen, anstatt eine breite Prüfung zu verwenden?“
Struktur
„Diese Funktion scheint mehrere Dinge zu tun. Können wir die Verantwortlichkeiten trennen?“
„Würde es das Testen und Reviewen erleichtern, dies in kleinere Funktionen aufzuteilen?“
Grenzfälle
„Was passiert, wenn dieser Schlüssel fehlt?“
„Sollten wir die Eingabe vor der Verarbeitung validieren?“
PR-Qualität
„Könnten Sie eine kurze PR-Beschreibung hinzufügen, die die Änderung erklärt und wie sie getestet werden sollte?“

Live-Demo: Prüfen Sie diesen Pull Request

Ein absichtlich fehlerhafter PR zum Üben. Können Sie die Probleme finden?

Branch: feature/cart-total-update
Titel: "Update cart calculation logic"
Beschreibung: "Updated the cart calculation logic and added support for discount."
src/cart.py
def calc(items, user, d=False): t = 0 for i in items: if i["type"] == "book": t += i["price"] * 0.9 else: t += i["price"] if d: t = t - t * 0.1 if user is not None: if "is_premium" in user: if user["is_premium"] == True: t = t - t * 0.05 return t def process_cart(items, user): total = calc(items, user, True) print("cart processed") return total
tests/test_cart.py
from src.cart import calc def test_calc(): items = [{"type": "book", "price": 100}] user = {"is_premium": True} assert calc(items, user, True) > 0
README.md
# Cart Project cart logic
Review-Leitfaden - Zu findende Probleme
Probleme auf PR-Ebene
  • Vager PR-Titel: „Update cart calculation logic“ beschreibt nicht, was geändert wurde
  • Die Beschreibung ist allgemein und nicht hilfreich
  • Kein Testplan oder Anweisungen für Reviewer
Probleme auf Code-Ebene
  • Funktionsname calc ist zu allgemein - sollte calculate_cart_total sein
  • Parameter d ist unklar - sollte discount_enabled sein
  • Variablen t und i sind nicht aussagekräftig
  • Magische Zahlen: 0.9, 0.1, 0.05 sollten benannte Konstanten sein
  • Tief verschachtelte if-Blöcke für Premium-Prüfung - verwenden Sie .get()
  • process_cart hat einen Seiteneffekt (print) gemischt mit Geschäftslogik
  • Keine Eingabevalidierung für items oder user
  • Boolscher Parameter d=False, der positionell als True übergeben wird, ist verwirrend
Probleme auf Test-Ebene
  • Nur ein Testfall für das gesamte Modul
  • Schwache Assertion: > 0 überprüft nicht die Korrektheit
  • Keine Grenzfall-Tests (leere Liste, fehlende Schlüssel, Nicht-Premium-Benutzer)
  • Testet process_cart nicht
Probleme auf Repo-Ebene
  • README ist im Wesentlichen leer - keine Einrichtungsanweisungen, keine Beschreibung
  • Keine Informationen darüber, wie Tests ausgeführt werden
Verbesserte Version - cart_improved.py
src/cart_improved.py
BOOK_DISCOUNT_RATE = 0.10 GENERAL_DISCOUNT_RATE = 0.10 PREMIUM_DISCOUNT_RATE = 0.05 def calculate_item_price(item): item_type = item["type"] price = item["price"] if item_type == "book": return price * (1 - BOOK_DISCOUNT_RATE) return price def apply_general_discount(total_price, discount_enabled): if not discount_enabled: return total_price return total_price * (1 - GENERAL_DISCOUNT_RATE) def apply_premium_discount(total_price, user): if not user or not user.get("is_premium", False): return total_price return total_price * (1 - PREMIUM_DISCOUNT_RATE) def calculate_cart_total(items, user, discount_enabled=False): total_price = 0 for item in items: total_price += calculate_item_price(item) total_price = apply_general_discount(total_price, discount_enabled) total_price = apply_premium_discount(total_price, user) return total_price def process_cart(items, user): total_price = calculate_cart_total(items, user, discount_enabled=True) return total_price

Hausaufgabe

PR-Review-Aufgabe: Reviewen Sie wie ein professioneller Ingenieur

Ihre Abgabe muss enthalten:

  • 10 gesamt Mindestens Review-Kommentare
  • 3 min Kommentare zur Benennung
  • 2 min Kommentare zum Testen
  • 2 min Kommentare zu Design oder Funktionsverantwortlichkeit
  • 1 min Kommentar zur Repository-Qualität
  • 1 min Refactoring-Vorschlag

Abgabeformat

File: src/orders.py
Line/Area: calc function
Comment: The function name is too generic. Could we rename
it to calculate_order_total for clarity?

File: tests/test_orders.py
Line/Area: test_calc
Comment: This test only checks that the result is greater
than zero. Could we assert the exact expected value instead?

Prüfen Sie die folgenden Dateien:

src/orders.py
def calc(items, vip=False): total = 0 for i in items: if i["type"] == "book": total += i["price"] * 0.9 elif i["type"] == "electronics": total += i["price"] else: total += i["price"] if vip == True: total = total - total * 0.05 return total def checkout(items, user): total = calc(items, user["vip"]) print("checking out order...") print("total is", total) return {"ok": True, "total": total}
src/utils.py
def f(x): if x == None: return False return True
tests/test_orders.py
from src.orders import calc def test_calc(): items = [{"type": "book", "price": 100}] assert calc(items, False) > 0
README.md
# Orders Run it with python.
Großartige Ingenieure bringen Code nicht nur zum Laufen. Sie machen Code verständlich, testbar, überprüfbar und vertrauenswürdig.
Zitat des Tages
„Der Code, den du heute zu beschäftigt bist aufzuräumen, wird morgen zum Problem, das dein Team beherrscht.“