Code Reviews

Devoxx Morocco, 2016

Transcript

1. Code Reviews Rabea Gransberger @rgransberger rgra.github.io
2. About Me Rabea Gransberger • Computer Science Diploma 2008 • Developer, Project Lead at MEKO-S GmbH, Bremen –Code Review supported by tools in all projects • Co-Organizer JUG Bremen 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
3. Agenda • Why do Code Reviews? • How to do Reviews? • Which Tools are available? • Tips for Developers and Reviewers • Which social problems can occur? 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
4. INTRODUCTION 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
5. Why Code Reviews? • Find errors • Increase customer satisfaction • Pareto principle (80/20 rule) • Quality of code • Education for whole team • Less stress • Lower overall project cost 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
6. Project cost [10] Bugs Cost After Development (463) QA/Test 142 200$ / fix Customer (within 6 month) 127 1000$ / fix Cost of fixing bugs 155k $ + Cost of 194 latent bugs 194k $ Total 349k $ 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
7. Project cost Bugs Cost After Development (463) Code Review 283 25$ / fix After QA/Test 67 200$ / fix After Customer 81 1000$ / fix Cost of fixing bugs 101k $ + Cost of 32 latent bugs 32k $ Total 133k $ (349 k $) 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
8. We already do TDD… • Readable code? • Errors are not only found in code: –Requirements –Design –Documentation –Test cases [3] 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
9. 24.10.2016 Codemotion Berlin / Code Reviews (Rabea Gransberger @rgransberger) https://lol.browserling.com/full-stack-hires.png
10. PROCESS & TECHNIQUES 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
11. Process Types •Formal –Inspection: formal meeting with whole team –Audit: by external company •Informal / Lightweight – Pair Programming: 2 developers, 1 keyboard – Walkthrough: Author shows code to Reviewer – Tool-supported Review •20 % time, same number issues 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
12. Example: Task based review process Roles: Author / Reviewer 1-* 24.10.2016 Codemotion Berlin / Code Reviews (Rabea Gransberger @rgransberger) Task Implement Review Request Review -1 Reopen +1 Done Review Comments Changes 1 - *
13. Pre-Requisites • Process backed by Team and Management • Deal with criticism: Code quality is important • Define standards: Syntax, naming, frameworks • Comprehensible tasks • Developers review own code before commit • Define goals 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
14. A CODE REVIEW EXAMPLE 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
15. public class ReviewCodeExample { public static BigDecimal FAC = new BigDecimal(0.1); public Collection<String> getCarNames() { List<Car> cars = getCarsFromDatabase(); List<String> carNames = new ArrayList<>(); for (Car car : cars) { if (!carNames.contains(car)) carNames.add(car.getName()); } return carNames; } } 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
16. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
17. Who? •Recommended: Every developer •How many reviewers per request? –min. 2 with different focus –Recommended: Expert in domain of review [1] 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
18. How? • Read task and extract requirements • Overview: What has changed? • Have requirements been met? • Check if code works by testing • Inspect code line by line • Identify issues, write comment and give priority • Difficult: Identify missing parts • Go slowly: 1 line changes at least 5min review 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
19. How: Eye Tracking 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
20. When? • Shortly after development/request • Pre-Commit or Post-Commit • Don’t postpone to day before release • Maximum 90 min per review 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
21. What? •Deviation from standard/requirements/code guidelines •Code has to be readable. Prefer refactoring to comment •Use small Checklists Book (Java): T. Gee: What to Look for in a Code Review (2016) 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
22. What? • Deviation from standard/requirements/code guidelines • Code has to be readable. Prefer refactoring to comment • Check coverage of new constants in if/switch • if without else • Correctness of exception handling • Prefer immutable objects • Spell check messages shown to users • synchronized/transactions for atomic operations • Watch out for Strings/Magic Numbers. Prefer value objects Book (Java): T. Gee: What to Look for in a Code Review (2016) 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
23. Example Checklist 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. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger) 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. Everything? • Just get started, every review helps • Start with high risk changes: –Change in important calculations –Safety critical code, e.g. authentication –Code without test coverage –Code of new team members –Change sets with high number of files touched 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
25. SMALL TOOLS 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
26. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
27. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
28. FindBugs • Static code analysis • Explanation with possible solution –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. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
29. Automated Review • Errors which can easily get overlooked –Naming and formatting –Wrong API usage (BigDecimal example) • Run before manual review –Developer before commit –Build-System/Continuous Integration • Important: Handling of False-Positives –FindBugs @SuppressFBWarnings 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger) Free to use: • FindBugs • Checkstyle • PMD • JQAssistant • SonarQube Payed: • Coverity
30. TOOL-BASED REVIEW 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
31. Example: GitHub Pull Request • Web-based Review • Commit/Branch/Task-based Review • Fork project / create branch / edit file on master • Create Pull Request • Notification for Repo Owners • Can add (line based) review comments on files • Close or accept pull request 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
32. GitHub Fork/Branch • Fork project on GitHub or create branch 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
33. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
34. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger) • Fork project on GitHub or create branch • Change file
35. • Mail: Notification Pull Request / Review • Web: Pending Pull Request / Review 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger) Pull Request Review
36. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
37. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
38. Review Tools • Reviews in pull requests Github • JetBrains Upsource * • Atlassian Crucible • Gerrit • Review Board • Phabricator Differential • SmartBear Collaborator / CodeReviewer* • ReviewClipse* (* with IDE integration) 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
39. Tools Checklist • Automatic Review creation via hooks from SCM • Adding new changes to existing review • Pre-/Post-Commit Review Support • Patch/Live-Code • Where are comments saved? Embedded in code, separate XML/Database? • Overview with all pending reviews • Tracking which code still needs review • Comments and priorities and possibility to mark comment as closed • Webpage / IDE Integration • Notifications by mail • Review by task / whole code base supported • Statistics to check effects of review / improve process 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
40. Example: Tools in your IDE •IDE provides sufficient support for reviews •SCM Integration •Issue Tracker Integration •Task Tags •Example: Review at MEKOS with Eclipse/RTC 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
41. Rational Team Concert 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
42. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
43. Rational Team Concert • Reviewer can query pending reviews • Select Work-Item with double click • Open attached Change-Sets to review code 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
44. RTC: Change Summary 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger) Open current code Changed files Commit comment
45. RTC: Diff View 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger) After Before
46. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger) public class ReviewCodeExample { public static BigDecimal FAC = new BigDecimal(0.1); public Collection<String> getCarNames() { List<Car> cars = getCarsFromDatabase(); List<String> carNames = new ArrayList<>(); for (Car car : cars) { //FIXME 4738 Use set instead of List if (!carNames.contains(car)) carNames.add(car.getName()); } return carNames; }
47. Review with Eclipse • Write comments in code • Prefix with Task-Tags TODO/FIXME + ID //TODO #4738 • Deliver comments with commit message “Review” • Review gets Rejected => Work Item Reopen 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
48. View Review Comments • Author gets notified about rejected review • Find comments with Eclipse View Tasks 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
49. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
50. Rework Author • Rewrite code and fix all comments • Remove task tag comments • Commit with comment „Rework Review“ • Work-Item to Verification state • Invite reviewer for next review 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger) Reviewer • All task tags removed • Re-Review code: • Changes between “Review” and “Rework” changesets
51. STATISTICS 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
52. Statistics Some review tools help to quantify positive effects of review Examples: • Issues by classification • Found issues • % reviewed code compared to full code base 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
53. Found Issues • Maintenance 71,7% –Naming, Comments 16,7 % –API Use/Formatting 13,0 % –Structure/Organisational 16,2 % –Solution Approach 20,6 % • Functional Problems 21,4% • False positives 7,5% x Industrial review, domain: Engineering, 9 Reviews, 1-4 Reviewer, 388 issues found [12] 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
54. PROCESS VARIATIONS 24.10.2016 Codemotion Berlin / Code Reviews (Rabea Gransberger @rgransberger)
55. Example: Task based review process Roles: Author / Reviewer 1-* 24.10.2016 Codemotion Berlin / Code Reviews (Rabea Gransberger @rgransberger) Task Implement Review Request Review -1 Reopen +1 Done Review Comments Changes 1 - *
56. Process embedding 24.10.2016 Codemotion Berlin / Code Reviews (Rabea Gransberger @rgransberger) Unit of work (IV-C1) • Release • Story/ Requirement • Task • Push/Pull/Comb. commit • Singular commit Trigger (IV-C2) • Tool • Conventions Publicness (IV-C3) • Pre-commit • Post-commit Unreviewed Release Prevention (IV-C4) • Organizational • Pre commit review • Release branch Swift completion (IV-C5) • Priority • WIP limit • Time slot • Author’s responsibility Blocking of process (IV-C6) • Full Follow-up • Wait for Review • No Blocking [20]
57. Reviewers 24.10.2016 Codemotion Berlin / Code Reviews (Rabea Gransberger @rgransberger) Rules Count/Skip (IV-D2) • Component • Author’s experience • Lifecycle phase • Change size • Pair programming • Reviewer’s choice • Author’s choice Population (IV-D3) • Everybody • Elite • Fixed Assignment (IV-D4) • Pull • Push • Mix • Fixed Assignment Tool (IV-D5) • No Tool • Reviewer Recommendation [20]
58. Checking 24.10.2016 Codemotion Berlin / Code Reviews (Rabea Gransberger @rgransberger) Interaction (IV-E1) • On-demand • Asynchronous Discussion • Meeting with author • Meeting without author Temporal Arrangement (IV-E2) • Parallel • Sequential Roles (IV-E3) • Yes • No Detection Aids (IV-E5) • Checklists • Static code analysis • Testing [20] Reviewer changes code (IV-E4) • Never • Sometimes
59. Feedback 24.10.2016 Codemotion Berlin / Code Reviews (Rabea Gransberger @rgransberger) Communication of issues (IV-F1) • Written • Oral only • Oral stored Handling of issues (IV-F2) • Resolve • Reject • Postpone • Ignore [20]
60. Overarching 24.10.2016 Codemotion Berlin / Code Reviews (Rabea Gransberger @rgransberger) Use of metrics (IV-G1) Metrics in use No metrics use Tool specialization (IV-G2) General-purpose Specialized [20]
61. TIPS & PRACTICAL EXPERIENCE 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
62. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
63. Tips for developers •Mistakes = Learn, don’t take personal! •Reviews don’t replace questions. Talk! •Refactoring in separate change set •Checklist review own changes before commit •Remind reviewer of important reviews •Reviewer isn’t necessarily right. Discuss 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
64. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
65. Tips for Reviewers •Make sure you are not disturbed •Prioritize if too many requests •Take time, don’t rush and accept •Don’t postpone reviews with many files •If you can’t test it, ask for walkthrough 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
66. Tips for Reviewers •Wrong! Provide advice on how to do better •Question don’t criticize. Don’t get personal! •Don’t fix code while reviewing (Bad fixes) •Praise good code and personal advances •Learn from team mates code 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
67. 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
68. Social Aspects •Reviews are unnecessary, they just cost time. •Process is boring •Author and reviewer get into conflict •Team members block process / approve fast 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
69. Social Aspects •Experience != Quality •Critique can cause depression •Big Brother Effect •Review gets rejected x-times 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
70. Code City Codetrails Code City Plugin 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
71. Related Tools / Concepts • Code Coverage • Code City / Code as a crime scene • Continuous Integration Server • Continuous Testing • Mutation testing • Random testinc 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
72. SUMMARY 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
73. Summary • Begin slowly & use existing tools • Define standards/checklists and use them • Configure tools for automated reviews • Create relaxed atmosphere • Reward: Less support calls / happy customers • Lowers overall project cost • Adjust process as you go 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)
74. Summary • Speak to each other • Every code review helps! 24.10.2016 Codemotion Berlin / Code Reviews (Rabea Gransberger @rgransberger)
75. Questions? Slides / Recordings: • http://rgra.github.io Contact information: • Rabea Gransberger (LinkedIn, Xing) • Twitter: @rgransberger Feedback welcome! 24.10.2016 Codemotion Berlin / Code Reviews (Rabea Gransberger @rgransberger)
76. Sources 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 Resourcen von Tobias Baum 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 20. A Faceted Classification Scheme for Change-Based Industrial Code Review Processes, T. Baum, 2016 03.11.2016 DevoxxMA / Code Reviews (Rabea Gransberger @rgransberger)