Upgrade to Pro
— share decks privately, control downloads, hide ads and more …
Speaker Deck
Features
Speaker Deck
PRO
Sign in
Sign up for free
Search
Search
The Art of Code Review
Search
John Cinnamond
May 09, 2016
Programming
4
670
The Art of Code Review
A talk about Code Review, given at LRUG in May 2016
John Cinnamond
May 09, 2016
Tweet
Share
More Decks by John Cinnamond
See All by John Cinnamond
Go Lift
jcinnamond
0
2.1k
Theory
jcinnamond
0
1.9k
The Point of Objects
jcinnamond
0
120
Complexity
jcinnamond
1
230
Other Decks in Programming
See All in Programming
Improving my own Ruby thereafter
sisshiki1969
1
160
@Environment(\.keyPath)那么好我不允许你们不知道! / atEnvironment keyPath is so good and you should know it!
lovee
0
120
Oracle Database Technology Night 92 Database Connection control FAN-AC
oracle4engineer
PRO
1
440
Ruby×iOSアプリ開発 ~共に歩んだエコシステムの物語~
temoki
0
320
Swift Updates - Learn Languages 2025
koher
2
480
Laravel Boost 超入門
fire_arlo
3
220
請來的 AI Agent 同事們在寫程式時,怎麼用 pytest 去除各種幻想與盲點
keitheis
0
120
Rancher と Terraform
fufuhu
2
400
モバイルアプリからWebへの横展開を加速した話_Claude_Code_実践術.pdf
kazuyasakamoto
0
330
rage against annotate_predecessor
junk0612
0
170
print("Hello, World")
eddie
2
530
Testing Trophyは叫ばない
toms74209200
0
880
Featured
See All Featured
Save Time (by Creating Custom Rails Generators)
garrettdimon
PRO
32
1.6k
Optimising Largest Contentful Paint
csswizardry
37
3.4k
Intergalactic Javascript Robots from Outer Space
tanoku
272
27k
GraphQLとの向き合い方2022年版
quramy
49
14k
Refactoring Trust on Your Teams (GOTO; Chicago 2020)
rmw
34
3.1k
RailsConf 2023
tenderlove
30
1.2k
VelocityConf: Rendering Performance Case Studies
addyosmani
332
24k
The Cult of Friendly URLs
andyhume
79
6.6k
Imperfection Machines: The Place of Print at Facebook
scottboms
268
13k
Chrome DevTools: State of the Union 2024 - Debugging React & Beyond
addyosmani
7
840
How STYLIGHT went responsive
nonsquared
100
5.8k
Visualizing Your Data: Incorporating Mongo into Loggly Infrastructure
mongodb
48
9.7k
Transcript
@jcinnamond THE ART OF CODE REVIEW
Code review is weird
We usually value… Collaboration Teamwork Removing distractions
You write some code Someone else comes along and points
out all your mistakes
S talkhub Talks Issues Pull requests Settings Actually, `Collaboration` and
`Teamwork` are saying the same thing search This talk
Code reviews are… Confrontational Egoistic Expensive Disruptive
REVIEWS VS HUGS 0 75 150 225 300 REVIEWS
So why bother?
Studies show code review can… Detect bugs Save money Improve
communication Prevent global warming Source: some company selling code review software
If you're going to use code review… …use it well
trunk feature Pull Request
John's Top Tips for Great Code Review Joy Treat with
a healthy dose of scepticism
Motivations
Expectations, Outcomes, and Challenges of Modern Code Review Alberto Bacchelli
& Christian Bird (2013) International Conference on Software Engineering, pages 712-721
"What are the motivations and expectations for modern code review?"
DEVELOPERS' MOTIVATIONS FINDING DEFECTS CODE IMPROVEMENT ALTERNATIVE SOLUTIONS KNOWLEDGE TRANSFER
0 100 200 300 400
John's Top Tip #1 Discuss the motivation as a team
CODE IMPROVEMENT UNDERSTANDING SOCIAL COMMUNICATION FINDING DEFECTS 0% 12.5% 25%
37.5% 50% Motivations Outcomes
John's Top Tip #2 Compare actual outcome with motivations
Creating a pull request
Think about the reviewer Do they know what I'm trying
to achieve? Are they aware of any constraints?
John's Top Tip #3 Write a description
In the description… Explain what the new feature is. Explain
why you made it. Suggest a path through the code.
John's Top Tip #4 Keep change size < 400 lines
Source: Cohen, Jason. (2006): The Best Kept Secrets of Peer Code Review.
Automated testing
John's Top Tip #5 Automatically run the test suite
John's Top Tip #6 Run linting tools (e.g., rubocop, hound)
Who should review?
To maximise defect detection, use 2 reviewers Source: Rigby, Peter
C, and Bird, Christian. (2013): Convergent Contemporary Software Peer Review Practices.
Who should review? Only the lead developer (Gatekeeper)
Who should review? Only senior developers (Gatekeepers, but more of
them)
Who should review? Developers at a similar level (True peer
review)
Who should review? Everyone
John's Top Tip #7 Get everyone involved in reviews (even
junior developers)
Reviewing code
Remember your motivation for reviewing
Valid comments (1) Problems with the code. i.e., actual defects
(not just things you don't like)
John's Top Tip #8 Explain the problem, don't criticise the
code
Valid comments (2) Questions about the code "I'm not sure
what this does"
S talkhub Talks Issues Pull requests Settings That isn't a
question? search This talk
Beware passive- aggressive questions Why did you use X here?
Why did you use X here - are you some kind of idiot?
Valid comments (2) Questions about the code "WWSMD?"
Valid comments (3) Improvements to the code "map would be
better than each"
John's Top Tip #9 Suggest improvements, don't dictate them
John's Top Tip #10 Justify your suggested improvements
Valid comments (4) Praise "this is nice! I'm going to
use this…"
Valid comments (5) Inconsistency with the rest of the codebase
Valid comments (6) Coding standard violations But only if there
is a standard to violate
John's Top Tip #11 Separate discussions on standards from code
review
Valid comments (7) There are no other valid comments. Code
review is not the place for this discussion.
Conversation is difficult via PR comments
John's Top Tip #13 Stop commenting. Talk.
John's Top Tip #14 Be yourself (unless you are a
monstrous pedant, in which case be someone better)
Example
def any_values?(hash) return hash.values.size >= 0 end
def any_values?(hash) return hash.values.size >= 0 end This is dumb
Doesn't explain the problem Feels bad, man
def any_values?(hash) return hash.values.size >= 0 end Never use return
☹ Why not? I'm being told off
def any_values?(hash) return hash.values.size >= 0 end Return is unnecessary
Maybe I should change it Your face is unnecessary
def any_values?(hash) return hash.values.size >= 0 end This is equivalent
to omitting the `return` The return is unnecessary
☹ def any_values?(hash) return hash.values.size >= 0 end The comparison
is wrong I did a wrong Why?
def any_values?(hash) return hash.values.size >= 0 end I think you
mean '> 0' Oops Go team!
def any_values?(hash) return hash.values.size >= 0 end This whole method
is pointless My work is pointless
def any_values?(hash) return hash.values.size >= 0 end What is the
point of this method? Why are they asking?
def any_values?(hash) return hash.values.size >= 0 end Why not ditch
this method and call `hash.values.any?` directly? …
def any_values?(hash) return hash.values.size >= 0 end <puts down pull
request, talks to developer?
The way you comment has a big impact
John's Top Tip #15 Avoid value judgements, even if they
about the code
Responding to comments
Try to address every comment
Try to create separate commits for each problem found
Tell the reviewer when you have addressed everything
Managing disagreement
John's Top Tip #16 If you disagree with any comments
talk about it
Don't reply with a new comment No good will come
from this
John's Top Tip #17 If you still disagree after talking,
ask someone else
Code review is inherently social Being "right" is not the
most important thing
Recap
Decide what you want from Code Review, as a team
1
Check that it actually delivers those benefits 2
Don't be shits to each other in pull requests 3
Thank you The Art of Code Review @jcinnamond LRUG 2016