Studies show code review
can…
Detect bugs
Save money
Improve communication
Prevent global warming
Source: some company selling code review
software
Slide 10
Slide 10 text
If you're going to use
code review…
…use it well
Slide 11
Slide 11 text
trunk
feature
Pull Request
Slide 12
Slide 12 text
John's Top Tips for
Great Code Review
Joy
Treat with a healthy dose of
scepticism
Slide 13
Slide 13 text
Motivations
Slide 14
Slide 14 text
Expectations, Outcomes, and
Challenges of Modern Code
Review
Alberto Bacchelli & Christian Bird
(2013) International Conference on Software Engineering, pages
712-721
Slide 15
Slide 15 text
"What are the motivations and
expectations for modern code
review?"
Slide 16
Slide 16 text
DEVELOPERS'
MOTIVATIONS
FINDING DEFECTS
CODE IMPROVEMENT
ALTERNATIVE SOLUTIONS
KNOWLEDGE TRANSFER
0 100 200 300 400
Slide 17
Slide 17 text
John's Top Tip #1
Discuss the
motivation as a
team
Slide 18
Slide 18 text
CODE IMPROVEMENT
UNDERSTANDING
SOCIAL COMMUNICATION
FINDING DEFECTS
0% 12.5% 25% 37.5% 50%
Motivations Outcomes
Slide 19
Slide 19 text
John's Top Tip #2
Compare actual
outcome with
motivations
Slide 20
Slide 20 text
Creating a
pull request
Slide 21
Slide 21 text
Think about the reviewer
Do they know what I'm trying to
achieve?
Are they aware of any constraints?
Slide 22
Slide 22 text
John's Top Tip #3
Write a
description
Slide 23
Slide 23 text
In the description…
Explain what the new feature is.
Explain why you made it.
Suggest a path through the code.
Slide 24
Slide 24 text
John's Top Tip #4
Keep change
size < 400 lines
Source: Cohen, Jason. (2006): The Best Kept
Secrets of Peer Code Review.
Slide 25
Slide 25 text
Automated
testing
Slide 26
Slide 26 text
John's Top Tip #5
Automatically
run the test
suite
Slide 27
Slide 27 text
John's Top Tip #6
Run linting tools
(e.g., rubocop, hound)
Slide 28
Slide 28 text
Who should
review?
Slide 29
Slide 29 text
To maximise defect
detection, use 2
reviewers
Source: Rigby, Peter C, and Bird, Christian.
(2013): Convergent Contemporary Software
Peer Review Practices.
Slide 30
Slide 30 text
Who should review?
Only the lead developer
(Gatekeeper)
Slide 31
Slide 31 text
Who should review?
Only senior developers
(Gatekeepers, but more of them)
Slide 32
Slide 32 text
Who should review?
Developers at a similar level
(True peer review)
Slide 33
Slide 33 text
Who should review?
Everyone
Slide 34
Slide 34 text
John's Top Tip #7
Get everyone
involved in
reviews
(even junior developers)
Slide 35
Slide 35 text
Reviewing
code
Slide 36
Slide 36 text
Remember your
motivation for
reviewing
Slide 37
Slide 37 text
Valid comments (1)
Problems with the code.
i.e., actual defects
(not just things you don't like)
Slide 38
Slide 38 text
John's Top Tip #8
Explain the
problem, don't
criticise the
code
Slide 39
Slide 39 text
Valid comments (2)
Questions about the code
"I'm not sure what this does"
Slide 40
Slide 40 text
S
talkhub
Talks Issues Pull requests Settings
That isn't a question?
search
This talk
Slide 41
Slide 41 text
Beware passive-
aggressive questions
Why did you use X here?
Why did you use X here - are you
some kind of idiot?
Slide 42
Slide 42 text
Valid comments (2)
Questions about the code
"WWSMD?"
Slide 43
Slide 43 text
Valid comments (3)
Improvements to the code
"map would be better than each"
Slide 44
Slide 44 text
John's Top Tip #9
Suggest
improvements,
don't dictate
them
Slide 45
Slide 45 text
John's Top Tip #10
Justify your
suggested
improvements
Slide 46
Slide 46 text
Valid comments (4)
Praise
"this is nice! I'm going to use
this…"
Slide 47
Slide 47 text
Valid comments (5)
Inconsistency with the rest of the
codebase
Slide 48
Slide 48 text
Valid comments (6)
Coding standard violations
But only if there is a standard to
violate
Slide 49
Slide 49 text
John's Top Tip #11
Separate
discussions on
standards from
code review
Slide 50
Slide 50 text
Valid comments (7)
There are no other valid
comments. Code review is not
the place for this discussion.
Slide 51
Slide 51 text
Conversation is difficult
via PR comments
Slide 52
Slide 52 text
John's Top Tip #13
Stop commenting.
Talk.
Slide 53
Slide 53 text
John's Top Tip #14
Be yourself
(unless you are a monstrous
pedant, in which case be
someone better)
Slide 54
Slide 54 text
Example
Slide 55
Slide 55 text
def any_values?(hash)
return hash.values.size >= 0
end
Slide 56
Slide 56 text
def any_values?(hash)
return hash.values.size >= 0
end
This is dumb
Doesn't explain the problem
Feels bad, man
Slide 57
Slide 57 text
def any_values?(hash)
return hash.values.size >= 0
end
Never use return
☹
Why not?
I'm being told off
Slide 58
Slide 58 text
def any_values?(hash)
return hash.values.size >= 0
end
Return is unnecessary
Maybe I should change it
Your face is unnecessary
Slide 59
Slide 59 text
def any_values?(hash)
return hash.values.size >= 0
end
This is equivalent to
omitting the `return`
The return is unnecessary
Slide 60
Slide 60 text
☹
def any_values?(hash)
return hash.values.size >= 0
end
The comparison is wrong
I did a wrong
Why?
Slide 61
Slide 61 text
def any_values?(hash)
return hash.values.size >= 0
end
I think you mean '> 0'
Oops
Go team!
Slide 62
Slide 62 text
def any_values?(hash)
return hash.values.size >= 0
end
This whole method is
pointless
My work is pointless
Slide 63
Slide 63 text
def any_values?(hash)
return hash.values.size >= 0
end
What is the point of this
method?
Why are they asking?
Slide 64
Slide 64 text
def any_values?(hash)
return hash.values.size >= 0
end
Why not ditch this method
and call `hash.values.any?`
directly?
…
Slide 65
Slide 65 text
def any_values?(hash)
return hash.values.size >= 0
end
Slide 66
Slide 66 text
The way you comment
has a big impact
Slide 67
Slide 67 text
John's Top Tip #15
Avoid value
judgements, even
if they about the
code
Slide 68
Slide 68 text
Responding
to comments
Slide 69
Slide 69 text
Try to address every
comment
Slide 70
Slide 70 text
Try to create separate
commits for each
problem found
Slide 71
Slide 71 text
Tell the reviewer when
you have addressed
everything
Slide 72
Slide 72 text
Managing
disagreement
Slide 73
Slide 73 text
John's Top Tip #16
If you disagree
with any
comments talk
about it
Slide 74
Slide 74 text
Don't reply with a new
comment
No good will come from this
Slide 75
Slide 75 text
John's Top Tip #17
If you still
disagree after
talking, ask
someone else
Slide 76
Slide 76 text
Code review is
inherently social
Being "right" is not
the most important
thing
Slide 77
Slide 77 text
Recap
Slide 78
Slide 78 text
Decide what you
want from Code
Review, as a team
1
Slide 79
Slide 79 text
Check that it
actually delivers
those benefits
2
Slide 80
Slide 80 text
Don't be shits to
each other in pull
requests
3
Slide 81
Slide 81 text
Thank you
The Art of Code Review
@jcinnamond
LRUG 2016