Code Review: Techniken und Tipps

JavaLand 2015

Transcript

2. Code Reviews: Techniken und Tipps Rabea Gransberger Dipl.-Inf. @rgransberger
3. Über mich Rabea Gransberger • Diplom Informatik 2008 (Universität Bremen) • Entwicklerin, Projekt- und Abteilungsleitung bei MEKO-S GmbH, Bremen, Germany • Hauptsächlich Projekte auf Basis von eclipse Technologie (RCP, RAP) • Code Reviews werden toolunterstützt in allen Projekten durchgeführt • Vorträge auf Konferenzen • Organisation JUG Bremen 3 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger)
4. Agenda • Warum macht man Code Reviews? • Wie laufen Reviews ab? • Welche Tools gibt es? • Welche Hürden gibt es? 5 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger)
5. EINLEITUNG 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 6
6. Warum Code Reviews? • Fehler finden • Kundenzufriedenheit • Paretoprinzip (80/20 Regel) • Qualität • Fortbildung • Weniger Stress • Geringere Projektkosten 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 7
7. Wirtschaftlicher Nutzen 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 8 [10]
8. Wirtschaftlicher Nutzen 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 9 [10]
9. Wir machen schon TDD… • Testfälle sichern nicht die Abwesenheit von Fehlern • Anforderungen nicht vollständig • Lesbarkeit / Verständlichkeit • Alleingang: 50% Effektivität im Finden von Fehlern • QA/Test: 70% • Review: 65% - 85% • Review + Test: 96% - 99% [3] 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 10
10. Übrigens… Fehler stecken nicht nur im produktiven Code Sondern auch in: • Anforderungen • Design • Dokumentation • Testfälle [3] 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 11
11. PROZESS & TECHNIKEN 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 12
12. Prozess Arten • Inspektion: formelle Review Sitzung mit dem Team • Audit: Durchführung durch externen Anbieter • Pair Programming: Zwei Autoren entwickeln an einem Rechner • Walkthrough/Over The Shoulder: Autor zeigt dem Reviewer den Code • Informell: u.a. Patches per Mail oder Toolbasiert 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 13
13. Task basierter Review Rollen • Autor • Reviewer 1-* 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 14 Aufgabe Implementierung Review- Anfrage Review -1 +1 Abschluss Review Kommentare
14. Voraussetzungen • Unterstützung durch Management und Team • Standards definieren: Syntax, Namenskonventionen, erlaubte Frameworks • Es geht um den Code / Qualität der Software • Kritikfähigkeit • Klare Aufgabenstellung / Verständnisprobleme vermeiden • Entwickler prüfen eigenen Code vor Commit • Ziele definieren 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 15
15. 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; } 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 16
16. 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 17
17. Wer? • Empfehlung: Jeder Entwickler • Wieviel Reviewer pro Anfrage? – Mindestens 2 mit unterschiedlichen Schwerpunkten [1] • Empfehlung: Experte in seinem Bereich [1] 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 18
18. Wie? • Geschwindigkeit kein Effizienzmaß – Rate Problemfunde sinkt > 450 LOC / Std. • Anforderungen aus Aufgabenstellung ermitteln • Überblick verschaffen – Welche Teile des System sind betroffen – Welche Codestellen wurden geändert/gelöscht? • Sind die Punkte der Anforderung erfüllt? • Funktioniert der Code / Oberflächen-Test • Gefundene Probleme werden in Kommentare gefasst und priorisiert • Am schwierigsten: Fehlende Teile identifizieren • Auch für Einzeiler min. 5 min Zeit 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 19
19. Wie: Eye Tracking 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 20
20. Wann? Zeitpunkt: • Pre-Commit • Post-Commit • Zeitnah nach Entwicklung • Nicht alles auf einmal vor Release • Maximal 90 Minuten per Review – Rate Problemfunde sinkt > 60 min 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 21
21. Was? Abweichungen von den Standards / Anforderungen / Clean Code • Code muss lesbar sein. Refactoring, notfalls Kommentar • Abdeckung neue Konstanten if/switch • if ohne else • Immutable Objekte präferieren • Prüfung Ausgaben Nachrichten/Fehlermeldung für Benutzer • Atomare Operationen in synchronized/transactions • Exception Handling korrekt • Verwendung von Strings/Magic Numbers • Build-Scripts etc. berücksichtigen 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 22
22. 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] 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 23
23. 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.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 24
24. KLEINE HELFERLEIN 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 25
25. Ein Beispiel 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 26
26. Die Auflösung 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 27
27. FindBugs • Statische Code-Analyse • Erklärung mit Lösungsvorschlägen Bug: Method de.rgransberger.review.examples.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. 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 28
28. Automatisierter Review • Findet Fehler die leicht übersehen werden – Probleme in Formatierung und Benennung – Fehlerhafte API Benutzung • Fokus auf inhaltliche Aspekte • Vor dem manuellen Review – Durch Entwickler vor Commit – Vom Build-System/Continuous Integration • Wichtig: Behandlung von False-Positives – z.B. FindBugs @SuppressFBWarnings 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 29
29. Automatisierter Review: Tools Kostenlos: • FindBugs: http://findbugs.sourceforge.net/downloads.html (Erweiterungen unter http://fb-contrib.sourceforge.net) • Checkstyle: http://checkstyle.sourceforge.net • PMD: http://pmd.sourceforge.net • SonarQube: http://www.sonarqube.org Kostenpflichtig u.a.: • Coverity: https://www.coverity.com 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 30
30. TOOLBASIERTER REVIEW 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 31
31. Beispiel: Atlassian Crucible 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 32
32. 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 33
33. 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 34 ID Beschreibung Dateien Zusammenfassung Allgemeine Kommentare Rollen Status Review Anweisungen
34. 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 35 Kommentar Klassifizierung Committer / Revision / Zeile Dateien
35. Weitere Tools • Review Board • Phabricator Differential • SmartBear Collaborator / CodeReviewer • Gerrit • JetBrains Upsource • Klocwork Cahoots 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 36
36. 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 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 37
37. 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 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 38
38. 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 39 ID Change-Sets Reviews Status
39. Rational Team Concert 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 40 Reviewer Review-Status
40. Rational Team Concert • Reviewer kann Liste anstehender Reviews abfragen • Auswahl Work-Item per Doppelklick • Öffnen der angehängten Change-Sets 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 41
41. 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 42 Nachher Vorher Dateien Aktuellen Code öffnen Commit-Kommentar
42. Review mit Eclipse • Markieren der Problemstellen durch 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 wird an das Work-Item mit Commit-Message „Review“ delivered • Review wird Rejected / Work Item Reopen 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 43
43. Tasks filtern • Autor erhält Benachrichtigung per Mail und in der IDE • Auffinden der Problemstellen in der Eclipse View Tasks 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 44
44. 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 45
45. Nacharbeiten • Überarbeitung und entfernen der Task Tags • Delivern mit „Nacharbeiten Review“ • Work-Item auf Verification und Reviewer zu neuem Review einladen Erneuter Review / Überprüfung: • Keine Task Tags mehr vorhanden • Anzeige aller nachgearbeiteten Code-Stellen: Markierung des Review-Changeset + folgender Changesets 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 46
46. STATISTIKEN 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 47
47. 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 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 48
48. 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 49
49. 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% 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 50
50. TIPPS & ERFAHRUNGEN 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 51
51. 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 52
52. Tipps für Entwickler • Kritik/Fehler = Lernen. Nicht persönlich nehmen! • Job erfordert laufende Fortbildung • Es gibt immer jemanden der mehr weiß • Review ersetzt keine Gespräche/Zwischenfragen/Diskussionen • Refactoring in neues Changeset • Vor dem Commit schauen ob alles im richtigen Changeset • Checkliste für eigenen Review vor Commit • Reviewer an Review erinnern, wenn wichtig für eigenen Zeitplan • Reviewer hat nicht immer Recht 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 53
53. Tipps für den Reviewer • Für Ruhe sorgen (kein klingelndes Telefon) • Zu viele Request: Nur die 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? • Wortwahl: Fragend statt tadelnd, nicht persönlich • Nicht den Code selber fixen (Bad fixes) • Guten Code/Persönliche Weiterentwicklung loben 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 54
54. 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 den Prozess / Unordentliche Nacharbeiten • Erfahrung != Qualität • Kritik wirft Personen aus der Bahn • Big Brother Effekt / Team fühlt sich überwacht • Review wird x-Mal rejected 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 55
55. 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 56
56. Code City 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 57 Codetrails Code City Plugin
57. Entfernte Tools • Code Coverage / Testabdeckung • Fehlermeldungen automatisch an Support senden • Code City / Code as a crime scene • Continuous Integration Server • Continuous Testing • Testdaten-Generatoren 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 58
58. FAZIT
59. Zusammenfassung • Langsam beginnen / Bereits vorhandene Tools nutzen • Standards/Checklisten definieren und Tools für automatischen Review konfigurieren • Entspannte Atmosphäre schaffen die Lernen in den Vordergrund stellt • Support-Stress nimmt ab / Kundenzufriedenheit zu • Zusatzaufwand lohnt sich auch wirtschaftlich • Prozess ist nicht in Stein gemeißelt sondern soll sich stetig verbessern Jeder einzelne Code Review hilft! 60 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger)
60. Fragen? Präsentationsmaterialien: https://speakerdeck.com/rgra Kontakt: Rabea Gransberger (Xing, LinkedIn, usw.) Twitter: @rgransberger Feedback Willkommen! 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 61
61. 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 24.03.2015 JavaLand 2015 / Code Reviews (Rabea Gransberger @rgransberger) 62