Making Code Reviews Beneficial for Everyone Involved

I gave this talk on 8/18 at Self Conference in Detroit.

Code reviews are a great opportunity for teams to help others grow, but giving and receiving these reviews can easily become one of the most frustrating parts of our jobs. It is easy for teams to develop bad habits that hurt our codebase and even end up inhibiting our growth as developers. How can we turn code reviews into a positive space that encourages improvement?

We will walk through (made up) code reviews and see how they can create problems on a team. We will discuss ways to turn these situations around and practical advice for language that can be used in code reviews. Come discuss some concrete ways to provide constructive and actionable feedback and learn the best ways to respond to code suggestions.

Transcript

1. Amanda Sopkin @amandasopkin amsopkin@gmail.com #devsum18 Making Code Reviews Beneficial for Everyone Involved
2. Where are we headed?
3. Code review “A systematic examination of computer code...intended to find mistakes overlooked in software development.”
4. Code reviews: Many different flavors
5. Improve us as developers Why do code reviews?
6. Improve us as developers Decentralize knowledge overall Why do code reviews?
7. Improve us as developers Decentralize knowledge Share responsibility Why do code reviews?
8. Improve us as developers Decentralize knowledge Share responsibility Improve code base quality Why do code reviews?
9. Why do code reviews? Improve us as developers Decentralize knowledge Share responsibility Improve code base quality Knowledge spreading
10. Costs of a mistake times less at code review 10 100 to
11. “The first code review” @amandasopkin
12. Should I know this? “The first code review” Despicable Idiot Outrageous WTF @amandasopkin
13. Levels of defense
14. Code Review Unit Testing Integration Testing Levels of defense
15. 55% Code Review Unit Testing Integration Testing Levels of defense 45% 30% @amandasopkin
16. Race conditions Memory leaks Threading issues Misunderstandings/assumptions Security problems What do code reviews catch?
17. Lightweight vs. Formal Code Reviews
18. Fagan inspection
19. Author (Coder) wrote the code Reader Reviewers Moderator Fagan inspection
20. Author (Coder) Reader paraphrases the task Reviewers Moderator Fagan inspection
21. Author (Coder) Reader Reviewers review the code Moderator Fagan inspection
22. Author (Coder) Reader Reviewers Moderator inspection/coaching Fagan inspection
23. Description what does this do Analysis Interpretation Evaluation Art critique Stages
24. Description Analysis how is it done Interpretation Evaluation Art critique Stages
25. Description Analysis Interpretation what could be better Evaluation Art critique Stages
26. Description Analysis Interpretation Evaluation or AND why Art critique Stages
27. Lightweight vs. formal
28. Over-the-shoulder Email pass-around Pair programming Tool assisted Lightweight code review
29. Over-the-shoulder Email pass-around Pair programming Tool assisted Lightweight code review
30. Over-the-shoulder Email pass-around Pair programming Tool assisted Lightweight code review Lightweight
31. Over-the-shoulder Email pass-around Pair programming Tool assisted Lightweight code review
32. Atlassian Crucible Atlassian's code review tool Google Gerrit Open source git code review tool Github Git hosting and pioneer of the "Pull Request" Facebook Phabricator Open source git/mercurial/svn review tool Review Board Open source review tool that is SCM/platform neutral JetBrains Upsource git/mercurial/perforce/svn review tool Visual Studio Team Services collaborative environment with git Code review tools
33. Code review tools: VSTS
34. Automated code review tools
35. Automated code review tools
36. Automated code review tools
37. Importance of human code review “Code Review is the single greatest way of noticing and killing bugs, increasing overall understanding, fixing design problems, and learning from one another.” -Graydon Hoare
38. 1. Functional 2. Clean and maintainable 3. Optimized Stages of Code Review
39. Hotbeds of bad dynamics
40. Hotbeds of bad dynamics anger jealousy mistrust confusion sad insecurity shame fear annoyed @amandasopkin
41. 5 Dysfunctions of a Team
43. Absence of trust Fear of conflict Lack of commitment Avoidance of accountability Inattention to results
44. Absence of trust Fear of conflict Lack of commitment Avoidance of accountability Inattention to results
45. Absence of trust Fear of conflict Lack of commitment Avoidance of accountability Inattention to results
46. Absence of trust Fear of conflict Lack of commitment Avoidance of accountability Inattention to results
47. Absence of trust Fear of conflict Lack of commitment Avoidance of accountability Inattention to results
49. Code Review example
50. feature/my-work: “Fixing a bug in our zoombinator” @amandasopkin
51. “This isn’t necessary. The zoombinator has this functionality built in.”
52. ? “The zoombinator is throwing a zoom exception and I thought this would fix the issue. I can take another look if you think that would be better.”
53. “It shouldn’t throw that exception. Are you sure you tested it correctly?”
54. “I think your solution is good, but I don’t want to butt in, so I won’t say anything.”
55. ? @amandasopkin
56. Dysfunctions at play in our situation
57. Mistrust
58. Fear of Conflict and Lack of Commitment
59. Avoidance of Accountability
60. Inattention to Results
61. 5 Counters for the 5 Dysfunctions of a Team
62. Absence of trust Fear of conflict Lack of commitment Avoidance of accountability Inattention to results
63. Absence of Trust -> Build Trust Fear of Conflict -> Be Honest and Open Lack of Commitment -> Work as a Team Avoidance of Accountability -> Be Accountable Inattention to Results -> Tune in
64. General strategy: Be empathetic
65. Be empathetic (Project Aristotle) People were skilled at reading emotions based on nonverbal cues. If a team member appeared uneasy with a decision, it was likely noticed and discussed. If someone appeared down, others showed concern and support. Those conversations are not always easy, but they’re important. They allow us to be authentic and engaged.
66. Be Empathetic AND Be Honest
67. Be Empathetic AND Be Honest
68. Absence of Trust Build Trust
69. Avoid assumptions
70. Share experiences
71. Point out the good
72. Build Trust Avoid assumptions Share your experiences with others Point out the good @amandasopkin
73. Fear conflict Be Honest and Open
74. Speak like you like each other
75. Speak like you like each other Make the other person feel safe The focus is NOT “being right” Say “What was the reasoning here...” instead of “why”
76. There is usually more than one way
78. Ask questions, not statements
79. Speak like you like each other There is usually more than one way to do it Ask questions, not statements Be Honest and Open @amandasopkin
80. Fear of Commitment Work as a Team
81. Explain things in the review I am changing this API because of a bug we discovered in XYZ steps.
82. After one hour, take a break
83. Do it thoroughly, or not at all! Initial comments Discussion Resolution Approval
84. Work as a Team Explain things in the review After one hour, stop and take a break Do it thoroughly or not at all @amandasopkin
85. Avoidance of Accountability Be Accountable
86. Break things up
87. Communicate early and often
88. Go face to face when necessary
89. Break things up in the review Communicate with others early and often Go face to face when necessary Be accountable @amandasopkin
90. Inattention to Results Tune in to the Team
91. Set a tone for the whole team
92. Establish guidelines/goals
93. Adheres to CodeStyle Proper object oriented design Sufficient test coverage Proper naming conventions Consider a checklist
94. Set a tone for the whole team Establish guidelines/goals Consider a checklist Tune in to the team @amandasopkin
95. General strategy: Be empathetic
96. Back to our earlier situation...
97. feature/my-work: “Fixing a bug in our zoombinator”
98. Inattention to Results Tune in to the Team
99. feature/my-work: “Fixing a bug in our zoombinator to prevent zoomification by sanitizing the request.”
100. “This isn’t necessary. The zoombinator has this functionality built in.”
101. Fear of Conflict Be Honest and Open
102. “Did you consider that the Zoombie class allows us to filter bad requests?”
103. ? “The zoombinator is throwing a zoom exception and I thought this would fix the issue. I can take another look if you think that would be better.”
104. Lack of Commitment Work as a Team
105. ? “The zoombinator is throwing a zoom exception. I think this will fix the issue because it won’t be caught at Zoomification.”
106. “It shouldn’t throw that exception. Are you sure you tested it correctly?”
107. Absence of Trust Build Trust
108. “Could you show me how this exception occurred?”
110. “Ok sounds good. Thanks for walking me through it.”
111. “I think your solution is good, but I don’t want to butt in, so I won’t say anything.”
112. Avoidance of Accountability Be Accountable
113. “I think this is the right strategy because we noticed the bug is not getting past the Zoomifier.”
114. Inattention to Results
115. Inattention to Results Tune in to the Team
116. Inattention to Results
117. ?
119. Conclusion
120. Empathy is Essential @amandasopkin
121. @amandasopkin
122. Something to say? @amandasopkin
123. Questions?
124. Thank you! @amandasopkin
125. • Icons taken from flaticon.com • “The 5 Dysfunctions of a Team” by Patrick Lencioni • “Code Complete” by Steve McConnell • “Best Kept Secrets of Peer Code Review” by Jason Cohen • The New York Times • Medium Sources
126. More Examples
127. “Why are you using the Z class?”
128. “Could you clean up this validation provider?”
129. “Could you clean up this validation provider?”
130. Absence of trust Fear of conflict Lack of commitment Avoidance of accountability Inattention to results