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
630
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
220
Other Decks in Programming
See All in Programming
Symfony Mapper Component
soyuka
2
690
【re:Growth 2024】 Aurora DSQL をちゃんと話します!
maroon1st
0
750
Testing Assembly: Code or Low Code - Navigating the Test Automation Options
maaretp
0
110
Refactor your code - refactor yourself
xosofox
1
230
[JAWS-UG横浜 #76] イケてるアップデートを宇宙いち早く紹介するよ!
maroon1st
0
430
Full stack testing :: basic to basic
up1
1
930
Keeping it Ruby: Why Your Product Needs a Ruby SDK - RubyWorld 2024
envek
0
170
「Chatwork」Android版アプリを 支える単体テストの現在
okuzawats
0
170
PipeCDの歩き方
kuro_kurorrr
4
150
これが俺の”自分戦略” プロセスを楽しんでいこう! - Developers CAREER Boost 2024
niftycorp
PRO
0
180
StarlingMonkeyを触ってみた話 - 2024冬
syumai
3
260
Criando Commits Incríveis no Git
marcelgsantos
2
160
Featured
See All Featured
Embracing the Ebb and Flow
colly
84
4.5k
The Language of Interfaces
destraynor
154
24k
ピンチをチャンスに:未来をつくるプロダクトロードマップ #pmconf2020
aki_iinuma
111
49k
The World Runs on Bad Software
bkeepers
PRO
65
11k
RailsConf & Balkan Ruby 2019: The Past, Present, and Future of Rails at GitHub
eileencodes
132
33k
個人開発の失敗を避けるイケてる考え方 / tips for indie hackers
panda_program
95
17k
Documentation Writing (for coders)
carmenintech
66
4.5k
Code Reviewing Like a Champion
maltzj
520
39k
VelocityConf: Rendering Performance Case Studies
addyosmani
326
24k
Into the Great Unknown - MozCon
thekraken
33
1.5k
Music & Morning Musume
bryan
46
6.2k
BBQ
matthewcrist
85
9.4k
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