The Art of Code Review

A talk about Code Review, given at LRUG in May 2016

Transcript

1. @jcinnamond THE ART OF CODE REVIEW
2. Code review is weird
3. We usually value… Collaboration Teamwork Removing distractions
4. You write some code Someone else comes along and points out all your mistakes
5. S talkhub Talks Issues Pull requests Settings Actually, `Collaboration` and `Teamwork` are saying the same thing search This talk
6. Code reviews are… Confrontational Egoistic Expensive Disruptive
7. REVIEWS VS HUGS 0 75 150 225 300 REVIEWS
8. So why bother?
9. Studies show code review can… Detect bugs Save money Improve communication Prevent global warming Source: some company selling code review software
10. If you're going to use code review… …use it well
11. trunk feature Pull Request
12. John's Top Tips for Great Code Review Joy Treat with a healthy dose of scepticism
13. Motivations
14. Expectations, Outcomes, and Challenges of Modern Code Review Alberto Bacchelli & Christian Bird (2013) International Conference on Software Engineering, pages 712-721
15. "What are the motivations and expectations for modern code review?"
16. DEVELOPERS' MOTIVATIONS FINDING DEFECTS CODE IMPROVEMENT ALTERNATIVE SOLUTIONS KNOWLEDGE TRANSFER 0 100 200 300 400
17. John's Top Tip #1 Discuss the motivation as a team
18. CODE IMPROVEMENT UNDERSTANDING SOCIAL COMMUNICATION FINDING DEFECTS 0% 12.5% 25% 37.5% 50% Motivations Outcomes
19. John's Top Tip #2 Compare actual outcome with motivations
20. Creating a pull request
21. Think about the reviewer Do they know what I'm trying to achieve? Are they aware of any constraints?
22. John's Top Tip #3 Write a description
23. In the description… Explain what the new feature is. Explain why you made it. Suggest a path through the code.
24. John's Top Tip #4 Keep change size < 400 lines Source: Cohen, Jason. (2006): The Best Kept Secrets of Peer Code Review.
25. Automated testing
26. John's Top Tip #5 Automatically run the test suite
27. John's Top Tip #6 Run linting tools (e.g., rubocop, hound)
28. Who should review?
29. To maximise defect detection, use 2 reviewers Source: Rigby, Peter C, and Bird, Christian. (2013): Convergent Contemporary Software Peer Review Practices.
30. Who should review? Only the lead developer (Gatekeeper)
31. Who should review? Only senior developers (Gatekeepers, but more of them)
32. Who should review? Developers at a similar level (True peer review)
33. Who should review? Everyone
34. John's Top Tip #7 Get everyone involved in reviews (even junior developers)
35. Reviewing code
36. Remember your motivation for reviewing
37. Valid comments (1) Problems with the code. i.e., actual defects (not just things you don't like)
38. John's Top Tip #8 Explain the problem, don't criticise the code
39. Valid comments (2) Questions about the code "I'm not sure what this does"
40. S talkhub Talks Issues Pull requests Settings That isn't a question? search This talk
41. Beware passive- aggressive questions Why did you use X here? Why did you use X here - are you some kind of idiot?
42. Valid comments (2) Questions about the code "WWSMD?"
43. Valid comments (3) Improvements to the code "map would be better than each"
44. John's Top Tip #9 Suggest improvements, don't dictate them
45. John's Top Tip #10 Justify your suggested improvements
46. Valid comments (4) Praise "this is nice! I'm going to use this…"
47. Valid comments (5) Inconsistency with the rest of the codebase
48. Valid comments (6) Coding standard violations But only if there is a standard to violate
49. John's Top Tip #11 Separate discussions on standards from code review
50. Valid comments (7) There are no other valid comments. Code review is not the place for this discussion.
51. Conversation is difficult via PR comments
52. John's Top Tip #13 Stop commenting. Talk.
53. John's Top Tip #14 Be yourself (unless you are a monstrous pedant, in which case be someone better)
54. Example
55. def any_values?(hash) return hash.values.size >= 0 end
56. def any_values?(hash) return hash.values.size >= 0 end This is dumb Doesn't explain the problem Feels bad, man
57. def any_values?(hash) return hash.values.size >= 0 end Never use return ☹ Why not? I'm being told off
58. def any_values?(hash) return hash.values.size >= 0 end Return is unnecessary Maybe I should change it Your face is unnecessary
59. def any_values?(hash) return hash.values.size >= 0 end This is equivalent to omitting the `return` The return is unnecessary
60. ☹ def any_values?(hash) return hash.values.size >= 0 end The comparison is wrong I did a wrong Why?
61. def any_values?(hash) return hash.values.size >= 0 end I think you mean '> 0' Oops Go team!
62. def any_values?(hash) return hash.values.size >= 0 end This whole method is pointless My work is pointless
63. def any_values?(hash) return hash.values.size >= 0 end What is the point of this method? Why are they asking?
64. def any_values?(hash) return hash.values.size >= 0 end Why not ditch this method and call `hash.values.any?` directly? …
65. def any_values?(hash) return hash.values.size >= 0 end <puts down pull request, talks to developer?
66. The way you comment has a big impact
67. John's Top Tip #15 Avoid value judgements, even if they about the code
68. Responding to comments
69. Try to address every comment
70. Try to create separate commits for each problem found
71. Tell the reviewer when you have addressed everything
72. Managing disagreement
73. John's Top Tip #16 If you disagree with any comments talk about it
74. Don't reply with a new comment No good will come from this
75. John's Top Tip #17 If you still disagree after talking, ask someone else
76. Code review is inherently social Being "right" is not the most important thing
77. Recap
78. Decide what you want from Code Review, as a team 1
79. Check that it actually delivers those benefits 2
80. Don't be shits to each other in pull requests 3
81. Thank you The Art of Code Review @jcinnamond LRUG 2016