Code Review: Techniken und Tipps

JAX 2015, Mainz
Recording: https://jaxenter.de/codereviews-tipps-tools-und-techniken-26793

1. Code Reviews: Techniken und Tipps Rabea Gransberger @rgransberger
2. Code Reviews: Techniken & Tipps @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
3. Code Reviews: Techniken & Tipps @rgransberger Agenda • Warum macht man Code Reviews? • Wie laufen Reviews ab? • Welche Tools gibt es? • Welche Hürden gibt es? • Fragen (gerne via Twitter)
4. Code Reviews: Techniken & Tipps @rgransberger EINLEITUNG
5. Code Reviews: Techniken & Tipps @rgransberger Warum Code Reviews? • Fehler finden • Kundenzufriedenheit • Paretoprinzip (80/20 Regel) • Qualität • Fortbildung • Weniger Stress • Geringere Projektkosten
6. Code Reviews: Techniken & Tipps @rgransberger Wirtschaftlicher Nutzen [10] Bugs Cost After Development 463 After QA/Test 321 200$ / fix After Customer 194 1000$ / fix Cost of fixing bugs 174k $ + Cost of 194 latent bugs 194k $ Total 368k $
7. Code Reviews: Techniken & Tipps @rgransberger Wirtschaftlicher Nutzen Bugs Cost After Development 463 After Code Review 180 25$ / fix After QA/Test 113 200$ / fix After Customer 32 1000$ / fix Cost of fixing bugs 120k $ + Cost of 32 latent bugs 32k $ Total 152k $
8. Code Reviews: Techniken & Tipps @rgransberger Wir machen schon TDD… • Testfälle sichern nicht die Abwesenheit von Fehlern • Anforderungen nicht vollständig • Lesbarkeit / Verständlichkeit • Alleingang: 50% Effektivität Finden von Fehlern • QA/Test: 70% • Review: 65% - 85% • Review + Test: 96% - 99% [3]
9. Code Reviews: Techniken & Tipps @rgransberger Übrigens… Fehler stecken nicht nur im produktiven Code Sondern auch in: • Anforderungen • Design • Dokumentation • Testfällen [3]
10. Code Reviews: Techniken & Tipps @rgransberger PROZESS & TECHNIKEN
11. Code Reviews: Techniken & Tipps @rgransberger 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 oder Toolbasiert
12. Code Reviews: Techniken & Tipps @rgransberger Task basierter Review Rollen • Autor • Reviewer 1-* Aufgabe Implemen- tierung Review- Anfrage Review -1 +1 Review Kommentare
13. Code Reviews: Techniken & Tipps @rgransberger Voraussetzungen • Unterstützung durch Management und Team • Standards definieren: Syntax, Namenskonventionen, erlaubte Frameworks • Kritikfähigkeit: Es geht um den Code / Qualität der Software • Klare Aufgabenstellung / Verständnisprobleme vermeiden • Entwickler prüfen eigenen Code vor Commit • Ziele definieren
14. Code Reviews: Techniken & Tipps @rgransberger EIN BEISPIEL
15. Code Reviews: Techniken & Tipps @rgransberger 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; }
16. Code Reviews: Techniken & Tipps @rgransberger
17. Code Reviews: Techniken & Tipps @rgransberger Wer? • Empfehlung: Jeder Entwickler • Wieviel Reviewer pro Anfrage? – Min. zwei mit unterschiedlichen Schwerpunkten [1] – Empfehlung: Experte in seinem Bereich [1]
18. Code Reviews: Techniken & Tipps @rgransberger Wie? • Anforderungen aus Aufgabenstellung ermitteln • Überblick verschaffen: Was wurde geändert? • Sind die Punkte der Anforderung erfüllt? • Funktioniert der Code / Oberflächen-Test • Gefundene Probleme in Kommentare und priorisierien • Am schwierigsten: Fehlende Teile identifizieren • Geschwindigkeit kein Effizienzmaß • Auch für Einzeiler min. 5 min Zeit
19. Code Reviews: Techniken & Tipps @rgransberger Wie: Eye Tracking
20. Code Reviews: Techniken & Tipps @rgransberger Wann? Zeitpunkt: • Pre-Commit • Post-Commit • Zeitnah nach Entwicklung • Nicht alles auf einmal vor Release • Maximal 90 Minuten per Review
21. Code Reviews: Techniken & Tipps @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 • Prüfung Nachrichten/Fehlermeldung für Benutzer • Atomare Operationen in synchronized/transactions • Verwendung von Strings/Magic Numbers
22. Code Reviews: Techniken & Tipps @rgransberger Beispiel Checkliste 1. Documentation: All subroutines are commented in clear language. 2. Documentation: Describe what happens with corner-case input. 3. Documentation: Complex algorithms are explained and justified. 4. Documentation: Code that depends on non-obvious behavior in external libraries is documented with reference to external documentation. 5. Documentation: Units of measurement are documented for numeric values. 6. Documentation: Incomplete code is indicated with appropriate distinctive markers (e.g. “TODO” or “FIXME”). 7. Documentation: User-facing documentation is updated (online help, contextual help, tool-tips, version history). 8. Testing: Unit tests are added for new code paths or behaviors. 9. Testing: Unit tests cover errors and invalid parameter cases. 10. Testing: Unit tests demonstrate the algorithm is performing as documented. 11. Testing: Possible null pointers always checked before use. 12. Testing: Array indexes checked to avoid out-of-bound errors. 13. Testing: Don’t write new code that is already implemented in an existing, tested API. 14. 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]
23. Code Reviews: Techniken & Tipps @rgransberger Alles? • Langsam anfangen: Jeder Review hilft! • Höhere Priorität für riskante Änderungen – Änderungen an bestehenden Berechnungen – Sicherheitskritischer Code – Code ohne Testfälle – Code von neuen Teammitgliedern – Änderungen an vielen Dateien • Alles kann Pflicht sein
24. Code Reviews: Techniken & Tipps @rgransberger KLEINE HELFERLEIN
25. Code Reviews: Techniken & Tipps @rgransberger Ein Beispiel
26. Code Reviews: Techniken & Tipps @rgransberger
27. Code Reviews: Techniken & Tipps @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.
28. Code Reviews: Techniken & Tipps @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 • Wichtig: Behandlung von False-Positives – z.B. FindBugs @SuppressFBWarnings
29. Code Reviews: Techniken & Tipps @rgransberger Automatisierter Review: Tools • FindBugs: http://findbugs.sourceforge.net (Erweiterungen http://fb-contrib.sourceforge.net) • Checkstyle: http://checkstyle.sourceforge.net • PMD: http://pmd.sourceforge.net • JQAssistant: http://jqassistant.org • SonarQube: http://www.sonarqube.org Kostenpflichtig u.a.: • Coverity: https://www.coverity.com
30. Code Reviews: Techniken & Tipps @rgransberger TOOLBASIERTER REVIEW
31. Code Reviews: Techniken & Tipps @rgransberger Beispiel: Atlassian Crucible • Webbasiertes Code Review Tool • Task Basierter Review • Basiert auf Fisheye Repo Browser
32. Code Reviews: Techniken & Tipps @rgransberger
33. Code Reviews: Techniken & Tipps @rgransberger
34. Code Reviews: Techniken & Tipps @rgransberger Übersicht Dateien Details
35. Code Reviews: Techniken & Tipps @rgransberger Übersicht
36. Code Reviews: Techniken & Tipps @rgransberger
37. Code Reviews: Techniken & Tipps @rgransberger Dateien
38. Code Reviews: Techniken & Tipps @rgransberger
39. Code Reviews: Techniken & Tipps @rgransberger Weitere Tools • Pullrequest Review Github • Review Board • Phabricator Differential • SmartBear Collaborator / CodeReviewer • Gerrit • JetBrains Upsource • Klocwork Cahoots
40. Code Reviews: Techniken & Tipps @rgransberger Tools Checkliste • Automatische Übernahme/Hooks für SCM und 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
41. Code Reviews: Techniken & Tipps @rgransberger Beispiel: Tools der IDE • IDE bietet gute Unterstützung für Reviews • SCM Integration • Issue Tracker Integration • Task Tags • Beispiel: Review bei MEKO-S mit Eclipse/Rational Team Concert
42. Code Reviews: Techniken & Tipps @rgransberger Rational Team Concert
43. Code Reviews: Techniken & Tipps @rgransberger
44. Code Reviews: Techniken & Tipps @rgransberger Rational Team Concert • Reviewer kann Liste anstehender Reviews abfragen • Auswahl Work-Item per Doppelklick • Öffnen der angehängten Change-Sets
45. Code Reviews: Techniken & Tipps @rgransberger RTC: Change Summary
46. Code Reviews: Techniken & Tipps @rgransberger RTC: Diff Ansicht
47. Code Reviews: Techniken & Tipps @rgransberger Review mit Eclipse
48. Code Reviews: Techniken & Tipps @rgransberger Review mit Eclipse • Kommentare im Code • Task-Tags TODO/FIXME + Work-Item ID • Preferences -> Java -> Editor -> Templates //TODO #${active.workItem.id} ${cursor} todo Strg+Space => //TODO #4738 • Code/Kommentare werden an das Work-Item mit Commit-Message „Review“ delivered • Review wird Rejected / Work Item Reopen
49. Code Reviews: Techniken & Tipps @rgransberger Tasks filtern • Autor erhält Benachrichtigung per Mail/ in IDE • Problemstellen in der Eclipse View Tasks
50. Code Reviews: Techniken & Tipps @rgransberger
51. Code Reviews: Techniken & Tipps @rgransberger Nacharbeiten • Überarbeitung und entfernen der Task Tags • Delivern mit „Nacharbeiten Review“ • Work-Item auf Verification • Reviewer zu neuem Review einladen
52. Code Reviews: Techniken & Tipps @rgransberger Nacharbeiten Erneuter Review / Überprüfung: • Keine Task Tags mehr vorhanden • Anzeige aller nachgearbeiteten Code-Stellen: Markierung des Review-Changeset + folgender Changesets
53. Code Reviews: Techniken & Tipps @rgransberger STATISTIKEN
54. Code Reviews: Techniken & Tipps @rgransberger Statistiken Statistiken um den Nutzen quantifizieren zu können Beispiele: • Gefundene Probleme nach Klassifizierung • Geöffnete vs. geschlossene Probleme • % Code in Review aus eingechecktem Code
55. Code Reviews: Techniken & Tipps @rgransberger Statistik: Gefundene Probleme Industrieller Review, Domäne: Engineering, 9 Reviews, 1-4 Reviewer, 388 Probleme gefunden [12] • Wartungsprobleme 71,7% – Benennung 16,7 % – API Nutzung 8,0 % – Formatierung 7,0 % – Strukturierung 16,2 % – Lösungsansatz 20,6 % • Funktionale Probleme 21,4% – Ressourcennutzung 5,9 % – Validierung 5,9 % – Schnittstellen 3,6 % – Logik 3,0 % – Größere Probleme 2,8 % • Falsch Positivfunde 7,5%
56. Code Reviews: Techniken & Tipps @rgransberger TIPPS & ERFAHRUNGEN
57. Code Reviews: Techniken & Tipps @rgransberger
58. Code Reviews: Techniken & Tipps @rgransberger 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
59. Code Reviews: Techniken & Tipps @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
60. Code Reviews: Techniken & Tipps @rgransberger Review Tipps • Falsch, aber wie geht es besser? • Wortwahl: Fragend statt tadelnd, nicht persönlich • Nicht den Code selber fixen (Bad fixes) • Guten Code/Persönliche Weiterentwicklung loben
61. Code Reviews: Techniken & Tipps @rgransberger Hürdenlauf • Reviews sind unnötig und Kosten nur Zeit • Prozess ist zu langweilig/aufwändig • Personen geraten in Konflikt / 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
62. Code Reviews: Techniken & Tipps @rgransberger
63. Code Reviews: Techniken & Tipps @rgransberger Code City Codetrails Code City Plugin
64. Code Reviews: Techniken & Tipps @rgransberger Entfernte Tools • Code Coverage / Testabdeckung • Fehlermeldungen automatisch an Support • Code City / Code as a crime scene • Continuous Integration Server • Continuous Testing • Testdaten-Generatoren
65. Code Reviews: Techniken & Tipps @rgransberger 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 • Jeder einzelne Code Review hilft!
66. Code Reviews: Techniken & Tipps @rgransberger FAZIT
67. Code Reviews: Techniken & Tipps @rgransberger Fragen? Präsentationsmaterialien: • https://speakerdeck.com/rgra Kontakt: • Rabea Gransberger (Xing, LinkedIn, usw.) • Twitter: @rgransberger • Feedback Willkommen! (JAX App)
68. Code Reviews: Techniken & Tipps @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