Code Reviews: the Good, the Bad, and the Ugly

Code reviews are an amazing opportunity for teams to help one another grow and maintain a high bar of quality for their code bases overall. Unfortunately this opportunity is often squandered and both giving and receiving these reviews can easily become one of the most frustrating parts of our jobs. Come learn about tactics for improving the process on your team.

Transcript

1. Amanda Sopkin @amandasopkin amsopkin@gmail.com #devsum18 Code Reviews
2. Slides: http://tinyurl.com/codereviews @amandasopkin
3. Don’t forget to evaluate this session in the DevSum app! @amandasopkin
4. Where are we headed?
5. Code review “A systematic examination of computer code...intended to find mistakes overlooked in software development.”
6. Code reviews: Many different flavors
7. Improve us as developers Why do code reviews?
8. Improve us as developers Decentralize knowledge overall Why do code reviews?
9. Improve us as developers Decentralize knowledge Share responsibility Why do code reviews?
10. Improve us as developers Decentralize knowledge Share responsibility Improve code base quality Why do code reviews?
11. Why do code reviews? Improve us as developers Decentralize knowledge Share responsibility Improve code base quality Knowledge spreading
12. Costs of a mistake times less at code review 10 100 to
13. “The first code review” @amandasopkin
14. Should I know this? “The first code review” Despicable Idiot Outrageous WTF @amandasopkin
15. Code Review Unit Testing Integration Testing Levels of defense
16. Levels of defense
17. 55% Code Review Unit Testing Integration Testing Levels of defense 45% 30% @amandasopkin
18. Race conditions Memory leaks Threading issues Misunderstandings/assumptions Security problems What do code reviews catch?
19. What do code reviews catch?
20. Lightweight vs. Formal Code Reviews
21. Fagan inspection
22. Author (Coder) wrote the code Reader Reviewers Moderator Fagan inspection
23. Author (Coder) Reader paraphrases the task Reviewers Moderator Fagan inspection
24. Author (Coder) Reader Reviewers review the code Moderator Fagan inspection
25. Author (Coder) Reader Reviewers Moderator inspection/coaching Fagan inspection
26. Lightweight vs. formal
27. Over-the-shoulder Email pass-around Pair programming Tool assisted Lightweight code review
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 Lightweight
30. Over-the-shoulder Email pass-around Pair programming Tool assisted Lightweight code review
31. 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
32. Code review tools: VSTS
33. Automated code review tools
34. Automated code review tools
35. Automated code review tools
36. 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
37. 1. Functional 2. Clean and maintainable 3. Optimized Stages of Code Review
38. Hotbeds of bad feelings
39. Hotbeds of bad dynamics anger jealousy mistrust confusion sad insecurity shame fear annoyed @amandasopkin
40. 5 Dysfunctions of a Team
42. Absence of trust Fear of conflict Lack of commitment Avoidance of accountability Inattention to results
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
48. Code Review example
49. feature/my-work: “Fixing a bug in our zoombinator” @amandasopkin
50. “This isn’t necessary. The zoombinator has this functionality built in.”
51. ? “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.”
52. “It shouldn’t throw that exception. Are you sure you tested it correctly?”
53. “I think your solution is good, but I don’t want to butt in, so I won’t say anything.”
54. ? @amandasopkin
55. Dysfunctions at play in our situation
56. Mistrust
57. Fear of Conflict and Lack of Commitment
58. Avoidance of Accountability
59. Inattention to Results
60. 5 Counters for the 5 Dysfunctions of a Team
61. Absence of trust Fear of conflict Lack of commitment Avoidance of accountability Inattention to results
62. 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
63. General strategy: Be empathetic
64. 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.
65. Be Empathetic AND Be Honest
66. Absence of Trust Build Trust
67. Avoid assumptions
68. Share experiences
69. Follow through
70. Build Trust Avoid assumptions Share your experiences with others Follow through @amandasopkin
71. Fear conflict Be Honest and Open
72. Speak like you like each other
73. 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”
74. There is usually more than one way
76. Ask questions, not statements
77. 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
78. Fear of Commitment Work as a Team
79. Explain things in the review I am changing this API because of a bug we discovered in XYZ steps.
80. After one hour, take a break
81. Do it thoroughly, or not at all! Initial comments Discussion Resolution Approval
82. 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
83. Avoidance of Accountability Be Accountable
84. Break things up
85. Communicate early and often
86. Go face to face when necessary
87. Break things up in the review Communicate with others early and often Go face to face when necessary Be accountable @amandasopkin
88. Inattention to Results Tune in to the Team
89. Set a tone for the whole team
90. Establish guidelines/goals
91. Adheres to CodeStyle Proper object oriented design Sufficient test coverage Proper naming conventions Consider a checklist
92. Set a tone for the whole team Establish guidelines/goals Consider a checklist Tune in to the team @amandasopkin
93. General strategy: Be empathetic
94. Back to our earlier situation...
95. feature/my-work: “Fixing a bug in our zoombinator”
96. Inattention to Results Tune in to the Team
97. feature/my-work: “Fixing a bug in our zoombinator to prevent zoomification by sanitizing the request.”
98. “This isn’t necessary. The zoombinator has this functionality built in.”
99. Fear of Conflict Be Honest and Open
100. “Did you consider that the Zoombie class allows us to filter bad requests?”
101. ? “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.”
102. Lack of Commitment Work as a Team
103. ? “The zoombinator is throwing a zoom exception. I think this will fix the issue because it won’t be caught at Zoomification.”
104. “It shouldn’t throw that exception. Are you sure you tested it correctly?”
105. Absence of Trust Build Trust
106. “Could you show me how this exception occurred?”
108. “Ok sounds good. Thanks for walking me through it.”
109. “I think your solution is good, but I don’t want to butt in, so I won’t say anything.”
110. Avoidance of Accountability Be Accountable
111. “I think this is the right strategy because we noticed the bug is not getting past the Zoomifier.”
112. Inattention to Results
113. Inattention to Results Tune in to the Team
114. Inattention to Results
115. ?
117. Conclusion
118. Empathy is Essential @amandasopkin
119. @amandasopkin
120. Something to say? @amandasopkin
121. Thank you! @amandasopkin
122. And…. Last but not least – don’t forget to evaluate this session in the DevSum app! @amandasopkin
123. Slides: http://tinyurl.com/codereviews @amandasopkin
124. Something to say? @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