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
660
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.8k
The Point of Objects
jcinnamond
0
110
Complexity
jcinnamond
1
230
Other Decks in Programming
See All in Programming
GoのWebAssembly活用パターン紹介
syumai
3
10k
アンドパッドの Go 勉強会「 gopher 会」とその内容の紹介
andpad
0
250
deno-redisの紹介とJSRパッケージの運用について (toranoana.deno #21)
uki00a
0
140
型付きアクターモデルがもたらす分散シミュレーションの未来
piyo7
0
800
レガシーシステムの機能調査・開発におけるAI利活用
takuya_ohtonari
0
610
Elixir で IoT 開発、 Nerves なら簡単にできる!?
pojiro
1
150
Result型で“失敗”を型にするPHPコードの書き方
kajitack
4
130
Haskell でアルゴリズムを抽象化する / 関数型言語で競技プログラミング
naoya
17
4.9k
なぜ適用するか、移行して理解するClean Architecture 〜構造を超えて設計を継承する〜 / Why Apply, Migrate and Understand Clean Architecture - Inherit Design Beyond Structure
seike460
PRO
1
430
「Cursor/Devin全社導入の理想と現実」のその後
saitoryc
0
120
Gleamという選択肢
comamoca
6
760
データベースコネクションプール(DBCP)の変遷と理解
fujikawa8
1
270
Featured
See All Featured
Reflections from 52 weeks, 52 projects
jeffersonlam
351
20k
The Cost Of JavaScript in 2023
addyosmani
51
8.4k
The Myth of the Modular Monolith - Day 2 Keynote - Rails World 2024
eileencodes
26
2.8k
VelocityConf: Rendering Performance Case Studies
addyosmani
330
24k
Site-Speed That Sticks
csswizardry
10
650
Bash Introduction
62gerente
614
210k
Into the Great Unknown - MozCon
thekraken
39
1.9k
BBQ
matthewcrist
89
9.7k
Building a Modern Day E-commerce SEO Strategy
aleyda
41
7.3k
CSS Pre-Processors: Stylus, Less & Sass
bermonpainter
357
30k
Dealing with People You Can't Stand - Big Design 2015
cassininazir
367
26k
The Power of CSS Pseudo Elements
geoffreycrofte
77
5.8k
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