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
640
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
2k
Theory
jcinnamond
0
1.8k
The Point of Objects
jcinnamond
0
110
Complexity
jcinnamond
1
230
Other Decks in Programming
See All in Programming
DRFを少しずつ オニオンアーキテクチャに寄せていく DjangoCongress JP 2025
nealle
2
300
はじめてのIssueOps - GitHub Actionsで実現するコメント駆動オペレーション
tmknom
0
210
Jakarta EE meets AI
ivargrimstad
0
720
コードを読んで理解するko build
bells17
1
120
React 19アップデートのために必要なこと
uhyo
8
1.6k
運用しながらリアーキテクチャ
nealle
0
200
Your Architecture as a Crime Scene:Forensic Analysis @bastacon 2025 in Frankfurt
manfredsteyer
PRO
0
120
Datadog DBMでなにができる? JDDUG Meetup#7
nealle
0
160
Kotlinの開発でも AIをいい感じに使いたい / Making the Most of AI in Kotlin Development
kohii00
5
2k
Jakarta EE meets AI
ivargrimstad
0
750
GoとPHPのインターフェイスの違い
shimabox
2
220
オレを救った Cline を紹介する
codehex
15
14k
Featured
See All Featured
What's in a price? How to price your products and services
michaelherold
244
12k
Faster Mobile Websites
deanohume
306
31k
A Philosophy of Restraint
colly
203
16k
Music & Morning Musume
bryan
46
6.4k
Bootstrapping a Software Product
garrettdimon
PRO
307
110k
Bash Introduction
62gerente
611
210k
ReactJS: Keep Simple. Everything can be a component!
pedronauck
666
120k
Art, The Web, and Tiny UX
lynnandtonic
298
20k
個人開発の失敗を避けるイケてる考え方 / tips for indie hackers
panda_program
100
18k
Building Better People: How to give real-time feedback that sticks.
wjessup
367
19k
4 Signs Your Business is Dying
shpigford
183
22k
How to train your dragon (web standard)
notwaldorf
91
5.9k
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