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
いま中途半端なSwift 6対応をするより、Default ActorやApproachable Concurrencyを有効にしてからでいいんじゃない?
yimajo
2
440
株式会社 Sun terras カンパニーデック
sunterras
0
360
詳しくない分野でのVibe Codingで困ったことと学び/vibe-coding-in-unfamiliar-area
shibayu36
3
5.1k
Software Architecture
hschwentner
6
2.3k
AI Coding Meetup #3 - 導入セッション / ai-coding-meetup-3
izumin5210
0
3.4k
What's new in Spring Modulith?
olivergierke
1
160
Goで実践するドメイン駆動開発 AIと歩み始めた新規プロダクト開発の現在地
imkaoru
4
860
Leading Effective Engineering Teams in the AI Era
addyosmani
7
480
Claude CodeによるAI駆動開発の実践 〜そこから見えてきたこれからのプログラミング〜
iriikeita
0
300
All About Angular's New Signal Forms
manfredsteyer
PRO
0
190
他言語経験者が Golangci-lint を最初のコーディングメンターにした話 / How Golangci-lint Became My First Coding Mentor: A Story from a Polyglot Programmer
uma31
0
250
大規模アプリのDIフレームワーク刷新戦略 ~過去最大規模の並行開発を止めずにアプリ全体に導入するまで~
mot_techtalk
1
470
Featured
See All Featured
RailsConf & Balkan Ruby 2019: The Past, Present, and Future of Rails at GitHub
eileencodes
140
34k
JavaScript: Past, Present, and Future - NDC Porto 2020
reverentgeek
52
5.6k
Statistics for Hackers
jakevdp
799
220k
Become a Pro
speakerdeck
PRO
29
5.6k
A designer walks into a library…
pauljervisheath
209
24k
StorybookのUI Testing Handbookを読んだ
zakiyama
31
6.2k
CoffeeScript is Beautiful & I Never Want to Write Plain JavaScript Again
sstephenson
162
15k
Automating Front-end Workflow
addyosmani
1371
200k
個人開発の失敗を避けるイケてる考え方 / tips for indie hackers
panda_program
115
20k
Chrome DevTools: State of the Union 2024 - Debugging React & Beyond
addyosmani
9
910
Performance Is Good for Brains [We Love Speed 2024]
tammyeverts
12
1.2k
Bootstrapping a Software Product
garrettdimon
PRO
307
110k
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