Code Reviews: Techniken und Tipps

Code Reviews: Techniken und Tipps
Rabea Gransberger
@rgransberger
Über mich
Rabea Gransberger
• Diplom Informatik 2008
• Entwicklerin, Projekt- und
Abteilungsleitung bei MEKO-S GmbH, Bremen
–Projekt-Basis eclipse Technologie (RCP, RAP)
–Code Reviews toolunterstützt in allen Projekten
• Vorträge auf Konferenzen
• Organisationsteam JUG Bremen
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Agenda
• Warum macht man Code Reviews?
• Wie laufen Reviews ab?
• Welche Tools gibt es?
• Tipps für Entwickler und Reviewer
• Welche Hürden gibt es?
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
EINLEITUNG
Warum Code Reviews?
• Fehler finden
• Kundenzufriedenheit
• Paretoprinzip (80/20 Regel)
• Qualität des Code
• Fortbildung des ganzen Teams
• Weniger Stress
• Geringere Projektkosten
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Wirtschaftlicher Nutzen
After Development
After QA/Test
After Customer
Cost of fixing bugs
+ Cost of 194 latent bugs
Total
Bugs
463
321
194
Cost
200$ / fix
1000$ / fix
174k $
194k $
368k $
[10]
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Wirtschaftlicher Nutzen
After Development
After Code Review
After QA/Test
After Customer
Cost of fixing bugs
+ Cost of 32 latent bugs
Total
07.07.2016
Bugs
463
180
113
32
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Cost
25$ / fix
200$ / fix
1000$ / fix
120k $
32k $
152k $
Wir machen schon TDD…
• Lesbarkeit / Verständlichkeit?
• Fehler nicht nur im Code:
–Anforderungen
–Design / Architektur
–Dokumentation
–Testfälle
https://lol.browserling.com/full-stack-hires.png
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
PROZESS & TECHNIKEN
Prozess Arten
• Inspektion: formelle Review Sitzung mit Team
• Audit: Durchführung durch externen Anbieter
• Pair Programming: 2 Autoren an 1 Rechner
• Walkthrough: Autor zeigt Reviewer den Code
• Informell: u.a.
–Patches per Mail
–Toolunterstützter Review
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Task basierter Review
Beispiel-Ablauf:
Task
Implement
Review
Comments
Review
Request
Review
-1
Reopen
Rollen: Autor/ Reviewer 1-*
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
+1
Done
Voraussetzungen
• Unterstützung durch Management und Team
• Kritikfähigkeit: Es geht um den Code / Qualität der Software
• Standards definieren: Syntax, Namenskonventionen, Frameworks
• Klare Aufgabenstellung / Verständnisprobleme vermeiden
• Entwickler prüfen eigenen Code vor Commit
• Ziele definieren
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
EIN BEISPIEL
public class ReviewCodeExample {
public static String ID = "review.example";
public BigDecimal getFactor() {
return new BigDecimal(0.1);
}
public Collection<String> getCarNames() {
List<Car> cars = getCarsFromDatabase();
List<String> carNames = new ArrayList<>(cars.size());
for (Car car : cars) {
if (!carNames.contains(car))
carNames.add(car.getName());
}
return carNames;
}
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Wer?
• Empfehlung: Jeder Entwickler
• Wieviel Reviewer pro Anfrage?
–Min. zwei mit unterschiedlichen Schwerpunkten [1]
–Empfehlung: Experte in seinem Bereich [1]
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Wie?
• Anforderungen aus Aufgabenstellung ermitteln
• Überblick verschaffen: Was wurde geändert? Anforderungen erfüllt?
• Testfälle / Coverage ausführen
• Funktioniert der Code / Oberflächen-Test
• Code Zeile für Zeile durchgehen
• Probleme identifizieren, Kommentare schreiben und priorisieren
• Am schwierigsten: Fehlende Teile identifizieren
• Geschwindigkeit kein Effizienzmaß
• Auch für Einzeiler min. 5 min Zeit
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Wie: Eye Tracking
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Wann?
• Zeitnah nach Entwicklung
• Pre-Commit oder Post-Commit
• Nicht alles auf einmal vor Release
• Maximal 90 Minuten per Review
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Was?
• Abweichungen von Standards/Anforderungen/Clean Code
• Code muss lesbar sein. Refactoring, notfalls Kommentar
• Abdeckung neue Konstanten if/switch
• if ohne else
• Exception Handling korrekt
• Immutable Objekte präferieren
• Rechtschreibung/Grammatik in Nachrichten/Fehlermeldung für Benutzer
• Atomare Operationen in synchronized/transactions
• Verwendung von Strings/Magic Numbers
• Buch: Trisha Gee: What to Look for in a Code Review (2016)
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Beispiel Checkliste
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
Documentation: All subroutines are commented in clear language.
Documentation: Describe what happens with corner-case input.
Documentation: Complex algorithms are explained and justified.
Documentation: Code that depends on non-obvious behavior in external
libraries is documented with reference to external documentation.
Documentation: Units of measurement are documented for numeric values.
Documentation: Incomplete code is indicated with appropriate distinctive
markers (e.g. “TODO” or “FIXME”).
Documentation: User-facing documentation is updated (online help, contextual
help, tool-tips, version history).
Testing: Unit tests are added for new code paths or behaviors.
Testing: Unit tests cover errors and invalid parameter cases.
Testing: Unit tests demonstrate the algorithm is performing as documented.
Testing: Possible null pointers always checked before use.
Testing: Array indexes checked to avoid out-of-bound errors.
Testing: Don’t write new code that is already implemented in an existing, tested
API.
Testing: New code fixes/implements the issue in question.
15. Error Handling: Invalid parameter values are handled properly early in the
subroutine.
16. Error Handling: Error values of null pointers from subroutine invocations are
checked.
17. Error Handling: Error handlers clean up state and resources no matter where an
error occurs.
18. Error Handling: Memory is released, resources are closed, and reference
counters are managed under both error and nonerror conditions.
19. Thread Safety: Global variables are protected by locks or locking subroutines.
20. Thread Safety: Objects accessed by multiple threads are accessed only through a
lock.
21. Thread Safety: Locks must be acquired and released in the right order to prevent
deadlocks, even in error-handling code.
22. Performance: Objects are duplicated only when necessary.
23. Performance: No busy-wait loops instead of proper thread synchronization
methods.
24. Performance: Memory usage is acceptable even with large inputs.
25. Performance: Optimization that makes code harder to read should only be
implemented if a profiler or other tool has indicated that the routine stands to
gain from optimization.
[10]
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Alles?
• Langsam anfangen: Jeder Review hilft!
• Höhere Priorität für riskante Änderungen
–Änderungen an bestehenden Berechnungen
–Sicherheitskritischer Code, z.B. Authentifizierung
–Code ohne Testfälle
–Code von neuen Teammitgliedern
–Änderungen an vielen Dateien
• Alles kann Pflicht sein
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
KLEINE HELFERLEIN
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
FindBugs
• Statische Code-Analyse
• Erklärung mit Lösungsvorschlägen
–Bug: Method ReviewCodeExample.getFactor() passes double value to BigDecimal
Constructor
–This method calls the BigDecimal constructor that takes a double, and passes a
literal double constant value. Since the use of BigDecimal is to get better precision
than double, by passing a double, you only get the precision of double number
space. To take advantage of the BigDecimal space, pass the number as a string.
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Automatisierter Review
• Findet Fehler die leicht übersehen werden
–Probleme in Formatierung und Benennung
–Fehlerhafte API Benutzung
• Vor dem manuellen Review
–Durch Entwickler vor Commit
–Vom Build-System/Continuous Integration
Kostenlos:
• FindBugs:
(Extensions)
• Checkstyle:
• PMD:
• JQAssistant:
• SonarQube:
Lizenzpflichtig:
• Coverity:
• Wichtig: Behandlung von False-Positives
–z.B. FindBugs @SuppressFBWarnings
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
http://findbugs.sourceforge.net
http://fb-contrib.sourceforge.net)
http://checkstyle.sourceforge.net
http://pmd.sourceforge.net
http://jqassistant.org
http://www.sonarqube.org
https://www.coverity.com
TOOLBASIERTER REVIEW
Beispiel: GitHub Pull Request
• Webbasierter Code Review
• Commit/Branch/Task-basierter Review unterstützt
• Fork von Projekt / Branch erstellen / Datei in Master editieren
• Pull Request erstellen
• Repository Owner werden benachrichtigt
• Zeilenbasierte Kommentare können hinzugefügt werden
• Pull request kann zurückgewiesen oder akzeptiert werden
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Pull Request Review
• Mail: Notification Pull Request / Review
• Web: Pending Pull Request / Review
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Weitere Tools
• Reviews in pull requests Github (reviewable.io)
• JetBrains Upsource *
• Atlassian Crucible
• Gerrit / eGerrit *
• Review Board
• Phabricator Differential
• SmartBear Collaborator / CodeReviewer*
• ReviewClipse*
(* mit IDE Integration)
• Siehe auch: 20 Best Code Review Tools for Developers
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Tools Checkliste
•
•
•
•
•
•
•
•
•
•
•
•
Automatische Übernahme/Hooks für SCM
Aktualisierung bestehender Reviews
Pre-/Post-Commit Review Unterstützung
Patch/Live-Code
Wo werden Kommentare gespeichert? Code oder DB/XML?
Übersichtsseite anstehende Reviews
Tracking welche Codestellen man angeschaut hat
Kommentare mit Priorität und Thread-Aufbau
Extra Webseite und/oder IDE Integration
Benachrichtigungen per Mail
Review pro Task oder ganze Code Basis
Statistiken um Effektivität Review prüfen zu können
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Beispiel: Tools der IDE
• IDE bietet gute Unterstützung für Reviews
• SCM Integration
• Issue Tracker Integration
• Task Tags
• Beispiel: Review bei MEKOS mit Eclipse/Rational Team Concert
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Rational Team Concert
• Reviewer kann Liste anstehender Reviews abfragen
• Auswahl Work-Item per Doppelklick
• Öffnen der angehängten Change-Sets
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
RTC: Change Summary
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
RTC: Diff Ansicht
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Review mit Eclipse
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Review mit Eclipse
• Kommentare im Code
• Task-Tags TODO/FIXME + Work-Item ID
todo Strg+Space => //TODO #4738
• Code/Kommentare werden an das Work-Item mit Commit-Message
„Review“ delivered
• Review wird Rejected => Work Item Reopen
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Tasks filtern
• Autor erhält Benachrichtigung per Mail/ in IDE
• Problemstellen in der Eclipse View Tasks
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Nacharbeiten
Autor
• Überarbeitung anhand der Kommentare
• Entfernen der Task Tags
• Commit mit „Nacharbeiten Review“
• Work-Item auf Verification
• Reviewer zu neuem Review einladen
07.07.2016
Reviewer
• Alle Task Tags entfernt
• Code erneut reviewern:
• Änderungen zwischen „Review“ und
„Nacharbeiten“ commits
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
STATISTIKEN
Statistiken
Statistiken um den Nutzen quantifizieren zu können:
• Gefundene Probleme nach Klassifizierung
• Geöffnete vs. geschlossene Probleme
• % Code in Review aus eingechecktem Code
Zeiterfassung:
• Zeit für Implementierung / Nacharbeiten / Review
• Anzahl gefundene Bugs
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Statistik: Gefundene Probleme
• Wartungsprobleme
71,7%
–Benennung
–API Nutzung /Formatierung
–Strukturierung
–Lösungsansatz
• Funktionale Probleme
• Falsch Positivfunde
16,7 %
13,0 %
16,2 %
20,6 %
21,4%
7,5%
x
Industrieller Review, Domäne: Engineering, 9 Reviews, 1-4 Reviewer, 388 Probleme gefunden [12]
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
TIPPS & ERFAHRUNGEN
Tipps für Entwickler
• Kritik/Fehler = Lernen. Nicht persönlich nehmen!
• Job erfordert laufende Fortbildung
• Review ersetzt keine Zwischenfragen/Diskussionen
• Refactoring in neues Changeset
• Vor Commit Changesets prüfen
• Checkliste für eigenen Review vor Commit
• Reviewer an Review erinnern, wenn wichtig
• Reviewer hat nicht immer Recht
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Tipps für den Reviewer
• Für Ruhe sorgen (kein klingelndes Telefon)
• Zu viele Request: Nur mit Prio, Rest ggf. löschen
• Nicht kurz schauen und OK sagen
• Viele Changesets nicht vor sich herschieben
• Schlecht Testbar als Walkthrough
• Falsch, aber wie geht es besser?
• Nicht auf Kleinigkeiten fokussieren
• Wortwahl: Fragend statt tadelnd, nicht persönlich
• Nicht den Code selber fixen (Bad fixes)
• Guten Code/Persönliche Weiterentwicklung loben
• Vom Code anderer lernen
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Hürdenlauf
• Reviews sind unnötig und Kosten nur Zeit
• Prozess ist zu langweilig/aufwändig
• Personen geraten in Konflikt / sind sich nicht einig
• Personen blockieren Prozess / Unordentlich
• Erfahrung != Qualität
• Kritik wirft Personen aus der Bahn
• Big Brother Effekt / Team fühlt sich überwacht
• Review wird x-Mal rejected
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Entfernte Tools
• Code Coverage / Testabdeckung
• Fehlermeldungen automatisch an Support
• Code City / Code as a crime scene
• Continuous Integration Server
• Continuous Testing
• Testdaten-Generatoren
• Mutation Testing
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
FAZIT
Zusammenfassung
• Langsam beginnen / vorhandene Tools nutzen
• Standards/Checklisten definieren
• Tools für automatischen Review konfigurieren
• Entspannte Atmosphäre schaffen
• Support-Stress nimmt ab/Kundenzufriedenheit zu
• Zusatzaufwand lohnt sich auch wirtschaftlich
• Prozess ist nicht in Stein gemeißelt
• Zusätzlich Architektur Reviews einführen
• Jeder einzelne Code Review hilft!
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Fragen?
Präsentationsmaterialien:
• https://speakerdeck.com/rgra
Kontakt:
• Rabea Gransberger (Xing, LinkedIn, usw.)
• Twitter: @rgransberger
Feedback Willkommen!
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)
Quellen
1. Understanding Open Source Software Peer Review: Review Processes, Parameters and Statistical Models, and Underlying Behaviours
and Mechanisms, Rigby, Dissertation, 2011
2. Convergent Contemporary Software Peer Review Practices, Rigby, 2013
3. Software Quality in 2002: A survey of the state of the art, Capers Jones, 2002
4. IEEE Standard for Software Reviews and Audits, IEEE Std 1028™-2008
5. Modernizing The Peer Code Review Process, KLOCWORK, White Paper, 2010
6. Code Reviews should be the universal rule of serious Software Development, Chhabra, Blog, 2012
7. How to hold a more effective code review, Stellman & Green, Blog, 2008
8. Code Review in Four Steps, Hayes, Blog, 2014
9. 11 proven practices for more effective, efficient peer code review, Cohen, 2011
10. Best Kept Secrets of Peer Code Review, Cohen, Smart Bear Inc., 2006
11. Don’t waste time on Code Reviews, Bird, Blog, 2014
12. Code Review Defects, Mäntylä & Lassenius, 2007
13. The Ten Commandments of Egoless Programming, Atwood, Blog, 2006
14. Improve Quality and Morale: Tips for Managing the Social Effects of Code Review, Smartbear, 2011
15. Code Reviews in Practice: Characteristics and Influencing Factors, Baum, 2015, (publication pending)
16. What to Look for in a Code Review, Gee, 2016
17. 20 Best Code Review Tools for Developers, Blog, 2015
18. Effektiver Einsatz von Code Review, OIO, 2015
19. Technische Schulden in Architekturen erkennen und beseitigen, Dr. C. Lilienthal, 2016
07.07.2016
Java Forum Stuttgart / Code Reviews (Rabea Gransberger @rgransberger)