1
99:59:59,999 --> 99:59:59,999
Good afternoon RailsConf.

2
99:59:59,999 --> 99:59:59,999
I hope you're all doing great after lunch.

3
99:59:59,999 --> 99:59:59,999
Thanks so much for coming. 
I've very excited to be here.

4
99:59:59,999 --> 99:59:59,999
My name is Derek Prior. 
I'm a developer with thoughtbot...

5
99:59:59,999 --> 99:59:59,999
...and I'm here today to talk to you 
about code reviews, doing them well...

6
99:59:59,999 --> 99:59:59,999
...and what it means for the culture 
of your team when you are in the

7
99:59:59,999 --> 99:59:59,999
type of place that does them well.

8
99:59:59,999 --> 99:59:59,999
So, let's start with a show of hands.

9
99:59:59,999 --> 99:59:59,999
Just so I get an idea where everyone is at.

10
99:59:59,999 --> 99:59:59,999
How many of you are doing code reviews

11
99:59:59,999 --> 99:59:59,999
as a regular part of your day, 
every day already?

12
99:59:59,999 --> 99:59:59,999
Ok. And how many of you really 
enjoy doing them?

13
99:59:59,999 --> 99:59:59,999
Okay, a few less.

14
99:59:59,999 --> 99:59:59,999
And how many of you do them because
you feel like you have to?

15
99:59:59,999 --> 99:59:59,999
It's about equal. Okay.

16
99:59:59,999 --> 99:59:59,999
All the people who said they really
do enjoyed them also said

17
99:59:59,999 --> 99:59:59,999
they do them because they have to.
Okay.

18
99:59:59,999 --> 99:59:59,999
So, why is it that we do reviews in the first place?

19
99:59:59,999 --> 99:59:59,999
This is pretty easy, right?

20
99:59:59,999 --> 99:59:59,999
To catch bugs.

21
99:59:59,999 --> 99:59:59,999
We're going to have somebody look
at every individual line of code

22
99:59:59,999 --> 99:59:59,999
and we're going to find what's wrong.

23
99:59:59,999 --> 99:59:59,999
Not really.

24
99:59:59,999 --> 99:59:59,999
That's not why it is interesting, right?
I've been doing code reviews for

25
99:59:59,999 --> 99:59:59,999
over ten years now.
I used to hate them.

26
99:59:59,999 --> 99:59:59,999
And, they were the worst
part of my day.

27
99:59:59,999 --> 99:59:59,999
We did it just for compliance
documentation at one of my former jobs

28
99:59:59,999 --> 99:59:59,999
But now I think that code reviews are
one of the primary ways

29
99:59:59,999 --> 99:59:59,999
that I get better at my job 
every single day.

30
99:59:59,999 --> 99:59:59,999
So

31
99:59:59,999 --> 99:59:59,999
Yes, we're gonna have fewer bugs
in code that's been peer reviewed.

32
99:59:59,999 --> 99:59:59,999
than in code that 
has not been peer reviewed.

33
99:59:59,999 --> 99:59:59,999
But studies on this show that
the level of QA that we get

34
99:59:59,999 --> 99:59:59,999
out of code review, doesn't meet
the level of expectation that we have

35
99:59:59,999 --> 99:59:59,999
as developers and managers.

36
99:59:59,999 --> 99:59:59,999
So, that is, we think that by doing
code reviews we're getting this much QA

37
99:59:59,999 --> 99:59:59,999
when in reality we're getting 
somewhere down here.

38
99:59:59,999 --> 99:59:59,999
So, why is that?

39
99:59:59,999 --> 99:59:59,999
Well, the reason is 
when we do code reviews,

40
99:59:59,999 --> 99:59:59,999
we're looking at a slice of a change.

41
99:59:59,999 --> 99:59:59,999
We're looking at the git diff essentially.

42
99:59:59,999 --> 99:59:59,999
And we can catch syntactical issues,
or problems where you might be

43
99:59:59,999 --> 99:59:59,999
calling a method on nil.

44
99:59:59,999 --> 99:59:59,999
But we can't catch the really 
heinous stuff which happens when

45
99:59:59,999 --> 99:59:59,999
your whole system interacts 
and corrupts your data.

46
99:59:59,999 --> 99:59:59,999
So, code reviews are good for 
some level of bug catching

47
99:59:59,999 --> 99:59:59,999
but it's not the end-all be-all, right?

48
99:59:59,999 --> 99:59:59,999
So, what are they good for?
I already told you that I think

49
99:59:59,999 --> 99:59:59,999
that code reviews make me better every day, 
and I want you all to feel the same way

50
99:59:59,999 --> 99:59:59,999
Well, in 2013, Microsoft and the 
University of Lugano in Switzerland

51
99:59:59,999 --> 99:59:59,999
came out with this study: "Expectations,
Outcomes, and Challenges of Modern

52
99:59:59,999 --> 99:59:59,999
Code Review." So, in it, what they did was
look at various teams across Microsoft

53
99:59:59,999 --> 99:59:59,999
which is a huge organization - they have 
several teams with different

54
99:59:59,999 --> 99:59:59,999
uh, they have Sr. developers, 
Jr. developers, managers, everybody

55
99:59:59,999 --> 99:59:59,999
all working on a different product.

56
99:59:59,999 --> 99:59:59,999
They surveyed all these people to ask them
"What is it you get out of code review?

57
99:59:59,999 --> 99:59:59,999
"What do you like about it, what do you 
don't like about it?"

58
99:59:59,999 --> 99:59:59,999
When they were done surveying them,
they watched them do code review

59
99:59:59,999 --> 99:59:59,999
and asked them questions afterwards.

60
99:59:59,999 --> 99:59:59,999
And, finally, they looked at all of the 
feedback that was given.

61
99:59:59,999 --> 99:59:59,999
Every comment that was logged in their
code review system.

62
99:59:59,999 --> 99:59:59,999
And they manually classified it

63
99:59:59,999 --> 99:59:59,999
They said "This one has to do with a bug
that they found. This one has to do with

64
99:59:59,999 --> 99:59:59,999
a style comment. This one has to do with 
a possible alternative solution."

65
99:59:59,999 --> 99:59:59,999
So after doing all this work, 
what they found was that people

66
99:59:59,999 --> 99:59:59,999
consistently ranked finding bugs very high
as a reason for doing code reviews.

67
99:59:59,999 --> 99:59:59,999
But in the end, it was actually a lot less
about finding bugs than anyone thought.

68
99:59:59,999 --> 99:59:59,999
The chief benefit they saw from code 
review were Knowledge Transfer,

69
99:59:59,999 --> 99:59:59,999
Increased Team Awareness, and
Finding Alternative Solutions to problems

70
99:59:59,999 --> 99:59:59,999
That sounds a lot more interesting to me
than hunting through code looking for bugs.

71
99:59:59,999 --> 99:59:59,999
Through this process we can 
improve our team.

72
99:59:59,999 --> 99:59:59,999
One person involved in the study 
said this:

73
99:59:59,999 --> 99:59:59,999
"Code review is the discipline of
explaining your code to your peers

74
99:59:59,999 --> 99:59:59,999
that drive a higher standard of coding.

75
99:59:59,999 --> 99:59:59,999
I think the process is even more important
than the result."

76
99:59:59,999 --> 99:59:59,999
I really do like that last part
even though it's not on the slide, right?

77
99:59:59,999 --> 99:59:59,999
The process is more important
than the result. And we're gonna talk

78
99:59:59,999 --> 99:59:59,999
about that today. Just by going through
the process of code review the right way,

79
99:59:59,999 --> 99:59:59,999
we're going to be improving our team.
Regardless of the actual results

80
99:59:59,999 --> 99:59:59,999
that we're seeing on any individual change.

81
99:59:59,999 --> 99:59:59,999
But I also like the definition here
"the discipline of explaining your code

82
99:59:59,999 --> 99:59:59,999
to your peers." I tweak
it just a little bit to say

83
99:59:59,999 --> 99:59:59,999
rather than trying to explain it to them

84
99:59:59,999 --> 99:59:59,999
Code review is one of the few chances we
get to have a black-and-white conversation

85
99:59:59,999 --> 99:59:59,999
about a particular change. We often talk
at abstractions

86
99:59:59,999 --> 99:59:59,999
like when we come to conferences
like this, we talk in large abstractions

87
99:59:59,999 --> 99:59:59,999
about things. And those are really
comfortable conversations.

88
99:59:59,999 --> 99:59:59,999
In code review, we have to get down 
to the implementation details

89
99:59:59,999 --> 99:59:59,999
and talk about how we're 
going to apply these things.

90
99:59:59,999 --> 99:59:59,999
So, like so much else that we do,
it's a communication issue.

91
99:59:59,999 --> 99:59:59,999
If we get better improving code reviews,
then what we're really doing is improving

92
99:59:59,999 --> 99:59:59,999
the technical communication 
on our team

93
99:59:59,999 --> 99:59:59,999
So, the study also found that
those benefits I cited earlier

94
99:59:59,999 --> 99:59:59,999
were particularly true for teams that
had a strong code review culture.

95
99:59:59,999 --> 99:59:59,999
So, what does it mean to have a
strong code review culture?

96
99:59:59,999 --> 99:59:59,999
To me, it means that your entire team
has to embrace the process.

97
99:59:59,999 --> 99:59:59,999
Everybody has to be involved. It can't be
something that the senior developers

98
99:59:59,999 --> 99:59:59,999
do for the junior developers. 
It's a discussion.

99
99:59:59,999 --> 99:59:59,999
That's what we're after here.

100
99:59:59,999 --> 99:59:59,999
So, as I mentioned earlier, 
I've been doing code reviews

101
99:59:59,999 --> 99:59:59,999
for over ten years now. But only in the
last 2-3 have I started to see a real

102
99:59:59,999 --> 99:59:59,999
improvement in what I'm getting out of
them, and in myself because of them

103
99:59:59,999 --> 99:59:59,999
And so why is that?

104
99:59:59,999 --> 99:59:59,999
I think it's because I'm part of a strong,
code review culture at thoughtbot.

105
99:59:59,999 --> 99:59:59,999
And, at thoughtbot we often go to clients
of various sizes to help them with issues

106
99:59:59,999 --> 99:59:59,999
that they're having. And nobody comes to
us and says, "I really need help improving

107
99:59:59,999 --> 99:59:59,999
the code review in my team."
That never happens.

108
99:59:59,999 --> 99:59:59,999
But when we get on the ground
with those teams, we often find

109
99:59:59,999 --> 99:59:59,999
that there isn't a strong
code review culture.

110
99:59:59,999 --> 99:59:59,999
One of the challenges that we have
is how do we get people to have

111
99:59:59,999 --> 99:59:59,999
this culture around code reviews.

112
99:59:59,999 --> 99:59:59,999
And there are a lot of little rules that
we suggest following in our guides.

113
99:59:59,999 --> 99:59:59,999
But if you look at them, you can 
really boil them down

114
99:59:59,999 --> 99:59:59,999
to two key rules of engagement, 
two things that you can do today

115
99:59:59,999 --> 99:59:59,999
to start getting better at code 
reviews with your team.

116
99:59:59,999 --> 99:59:59,999
The first of them is something 
that you can do as an author.

117
99:59:59,999 --> 99:59:59,999
The second is something
that you can do as a reviewer

118
99:59:59,999 --> 99:59:59,999
when you're providing feedback.

119
99:59:59,999 --> 99:59:59,999
So first, as an author, 
what are we going to do

120
99:59:59,999 --> 99:59:59,999
to make sure that our code reviews
get off on the right foot?

121
99:59:59,999 --> 99:59:59,999
So, this quote here is from Gary 
Vaynerchuck and he was talking

122
99:59:59,999 --> 99:59:59,999
about social media marketing,
or something, not interesting.

123
99:59:59,999 --> 99:59:59,999
But, it's also applicable to code reviews.

124
99:59:59,999 --> 99:59:59,999
Believe it or not.

125
99:59:59,999 --> 99:59:59,999
So, in a code review, your content
is the code, it's the diff

126
99:59:59,999 --> 99:59:59,999
it's what you've changed.

127
99:59:59,999 --> 99:59:59,999
The context is why that's changed.

128
99:59:59,999 --> 99:59:59,999
Your code doesn't exist for its 
own benefit. It solves a problem.

129
99:59:59,999 --> 99:59:59,999
So the context is 
what problem is it solving.

130
99:59:59,999 --> 99:59:59,999
That study I cited earlier found that
insufficient context is one of the

131
99:59:59,999 --> 99:59:59,999
chief impediments to a quality review.

132
99:59:59,999 --> 99:59:59,999
So, as an author, we need to get, we need 
to know this and get out in front of it.

133
99:59:59,999 --> 99:59:59,999
We're gonna provide excellent context
around our changes.

134
99:59:59,999 --> 99:59:59,999
So let's have a look
at a real world example.

135
99:59:59,999 --> 99:59:59,999
This is a commit, or a pull request title,
that you might see come across your email.

136
99:59:59,999 --> 99:59:59,999
"Use type column first in
multi-column indexes"

137
99:59:59,999 --> 99:59:59,999
The specifics here aren't 
particularly important

138
99:59:59,999 --> 99:59:59,999
if you don't understand what this means.

139
99:59:59,999 --> 99:59:59,999
The problem here is that 
there is no why.

140
99:59:59,999 --> 99:59:59,999
So, I can look at this change
and I can say "Well, yeah, this changes

141
99:59:59,999 --> 99:59:59,999
the order of the indexes, or the order 
of columns on multi-column indexes

142
99:59:59,999 --> 99:59:59,999
But I can't tell you if that was the best
solution to the problem you're solving.

143
99:59:59,999 --> 99:59:59,999
I can't really learn anything from it.

144
99:59:59,999 --> 99:59:59,999
So, this is a loss. It's not interesting
for me to review, and it's just going to

145
99:59:59,999 --> 99:59:59,999
get a thumbs up and move on.

146
99:59:59,999 --> 99:59:59,999
Actually, what I would do with a change 
like this is comment and say,

147
99:59:59,999 --> 99:59:59,999
"Can you provide some more context here?"

148
99:59:59,999 --> 99:59:59,999
And a lot of times, what we find happens,
is somebody updates the pull request

149
99:59:59,999 --> 99:59:59,999
or adds a comment that says 
something like this.

150
99:59:59,999 --> 99:59:59,999
Right?

151
99:59:59,999 --> 99:59:59,999
So, okay, so I guess you guys 
think that's not better, which is true.

152
99:59:59,999 --> 99:59:59,999
That's not better.

153
99:59:59,999 --> 99:59:59,999
So, this is probably a link to Github 
or maybe it's a link to Geer or whatever

154
99:59:59,999 --> 99:59:59,999
your issue tracker is, right?
And a lot of people do this.

155
99:59:59,999 --> 99:59:59,999
They'll provide a short explanation and 
say, "For more details, see this ticket."

156
99:59:59,999 --> 99:59:59,999
But what you're doing there is you're
making the reviewer hunt for this context.

157
99:59:59,999 --> 99:59:59,999
If they click this issue, are they gonna 
see a good description of what you're doing?

158
99:59:59,999 --> 99:59:59,999
Probably not, right?

159
99:59:59,999 --> 99:59:59,999
They're gonna see a report of a problem,
maybe some discussion back and forth

160
99:59:59,999 --> 99:59:59,999
about how to solve it, and then maybe
a link to this pull request or something.

161
99:59:59,999 --> 99:59:59,999
But they've got to go through all this
and they've got to hunt for that context

162
99:59:59,999 --> 99:59:59,999
and put it together until they are in the 
right frame of mind to review the changes,

163
99:59:59,999 --> 99:59:59,999
to tell you whether or not 
it is the right solution.

164
99:59:59,999 --> 99:59:59,999
So, here's an improvement.

165
99:59:59,999 --> 99:59:59,999
It's not important to read this, again.

166
99:59:59,999 --> 99:59:59,999
So, what we're doing is identifying 
the problem with index ordering.

167
99:59:59,999 --> 99:59:59,999
That's what we do first.

168
99:59:59,999 --> 99:59:59,999
Why is it a problem?

169
99:59:59,999 --> 99:59:59,999
We're gonna back it up with some 
postgres docs, and those link off to more

170
99:59:59,999 --> 99:59:59,999
information if you need it.
And because this particular change

171
99:59:59,999 --> 99:59:59,999
was a change to Rails, and we need
to be concerned with multiple adapters,

172
99:59:59,999 --> 99:59:59,999
we're also going to back it up with some 
MySQL documentation.

173
99:59:59,999 --> 99:59:59,999
And finally, we're gonna talk about 
why this is the best solution.

174
99:59:59,999 --> 99:59:59,999
So that's a lot of context.

175
99:59:59,999 --> 99:59:59,999
But it's important to note that now, as
somebody is coming along to review this,

176
99:59:59,999 --> 99:59:59,999
I know why this change is being made,
and maybe I'd even learn something about

177
99:59:59,999 --> 99:59:59,999
how multi-column indexes work, by reading
through some of this documentation.

178
99:59:59,999 --> 99:59:59,999
So there's value for me to review this.

179
99:59:59,999 --> 99:59:59,999
So, as an author, we're going to 
provide sufficient context.

180
99:59:59,999 --> 99:59:59,999
What we're trying to do here, like

181
99:59:59,999 --> 99:59:59,999
You've been working on this change for
four hours, two days, however long

182
99:59:59,999 --> 99:59:59,999
it took you to fix this, right?

183
99:59:59,999 --> 99:59:59,999
What you need to do is 
bring the reviewer up to speed.

184
99:59:59,999 --> 99:59:59,999
Let them know what you learned
in this process,

185
99:59:59,999 --> 99:59:59,999
and put them on equal footing with you.

186
99:59:59,999 --> 99:59:59,999
So I challenge you to provide
two paragraphs of context

187
99:59:59,999 --> 99:59:59,999
with every change you made.

188
99:59:59,999 --> 99:59:59,999
At first it's going to be really painful,
and there are some changes that it's

189
99:59:59,999 --> 99:59:59,999
torturous to describe in two paragraphs.

190
99:59:59,999 --> 99:59:59,999
And yes, it's going to take you more time.

191
99:59:59,999 --> 99:59:59,999
But how much more time?

192
99:59:59,999 --> 99:59:59,999
I don't know, like, five minutes, maybe?

193
99:59:59,999 --> 99:59:59,999
And it avoids a whole round of,
that round of questioning I described

194
99:59:59,999 --> 99:59:59,999
before, like "Why are you making this
change?" So we've headed that off.

195
99:59:59,999 --> 99:59:59,999
And the extra bonus is that all of that
context we saw earlier

196
99:59:59,999 --> 99:59:59,999
gets to live on in the commit.

197
99:59:59,999 --> 99:59:59,999
We're gonna squash that 
and we're gonna save that.

198
99:59:59,999 --> 99:59:59,999
So rather than that one we saw before
that had an issue - a link to G or a link

199
99:59:59,999 --> 99:59:59,999
to Github, that's going to go away
as soon as we stop paying that bill.

200
99:59:59,999 --> 99:59:59,999
But our git history is going to stay with 
us, and we're going to see that there.

201
99:59:59,999 --> 99:59:59,999
Ok, so that's what we can do
as an author.

202
99:59:59,999 --> 99:59:59,999
What about as a reviewer?

203
99:59:59,999 --> 99:59:59,999
What can we do to

204
99:59:59,999 --> 99:59:59,999
make it so that our 
feedback is well received?

205
99:59:59,999 --> 99:59:59,999
We like to call this
ask, don't tell.

206
99:59:59,999 --> 99:59:59,999
To start off with, it's 
important to note that

207
99:59:59,999 --> 99:59:59,999
research has shown there's a negativity
bias in written communication.

208
99:59:59,999 --> 99:59:59,999
That is to say if I have a conversation
with you face-to-face and I give you

209
99:59:59,999 --> 99:59:59,999
some feedback, or maybe 
even over the phone,

210
99:59:59,999 --> 99:59:59,999
just in a place where you can hear 
the tone of my voice, hear the inflection.

211
99:59:59,999 --> 99:59:59,999
You're gonna perceive that one way.

212
99:59:59,999 --> 99:59:59,999
If I give you that same feedback written,
like in the form of a pull request review,

213
99:59:59,999 --> 99:59:59,999
it's going to be perceived 
much more negatively

214
99:59:59,999 --> 99:59:59,999
So, it's important that 
we're cognizant of this

215
99:59:59,999 --> 99:59:59,999
negativity bias in our
written communication.

216
99:59:59,999 --> 99:59:59,999
We have to overcome this if we want
our feedback to be taken in the right way.

217
99:59:59,999 --> 99:59:59,999
One way to do that
is to offer compliments.

218
99:59:59,999 --> 99:59:59,999
I would suggest that you do that.

219
99:59:59,999 --> 99:59:59,999
So, if you find something in a 
pull request that is something you learned

220
99:59:59,999 --> 99:59:59,999
or is something that you think 
is done particularly well,

221
99:59:59,999 --> 99:59:59,999
those are great to call out.

222
99:59:59,999 --> 99:59:59,999
It lets everybody know that...

223
99:59:59,999 --> 99:59:59,999
"Yes, you taught me something. 
That's great. Thank you very much."

224
99:59:59,999 --> 99:59:59,999
Rather than just always nitpicking
at the change.

225
99:59:59,999 --> 99:59:59,999
But, there's gonna come a time when
you need to provide critical feedback