WEBVTT

99:59:59.999 --> 99:59:59.999
Good afternoon RailsConf.

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

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

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

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

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

99:59:59.999 --> 99:59:59.999
type of place that does them well.

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

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

99:59:59.999 --> 99:59:59.999
How many of you are doing code reviews

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

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

99:59:59.999 --> 99:59:59.999
Okay, a few less.

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

99:59:59.999 --> 99:59:59.999
It's about equal. Okay.

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

99:59:59.999 --> 99:59:59.999
they do them because they have to.
Okay.

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

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

99:59:59.999 --> 99:59:59.999
To catch bugs.

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

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

99:59:59.999 --> 99:59:59.999
Not really.

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

99:59:59.999 --> 99:59:59.999
over ten years now.
I used to hate them.

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

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

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

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

99:59:59.999 --> 99:59:59.999
So

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

99:59:59.999 --> 99:59:59.999
than in code that 
has not been peer reviewed.

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

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

99:59:59.999 --> 99:59:59.999
as developers and managers.

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

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

99:59:59.999 --> 99:59:59.999
So, why is that?

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

99:59:59.999 --> 99:59:59.999
we're looking at a slice of a change.

99:59:59.999 --> 99:59:59.999
We're looking at the git diff essentially.

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

99:59:59.999 --> 99:59:59.999
calling a method on nil.

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

99:59:59.999 --> 99:59:59.999
your whole system interacts 
and corrupts your data.

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

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

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

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

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

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

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

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

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

99:59:59.999 --> 99:59:59.999
all working on a different product.

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?

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

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

99:59:59.999 --> 99:59:59.999
and asked them questions afterwards.

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

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

99:59:59.999 --> 99:59:59.999
And they manually classified it

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

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

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

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

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

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

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

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

99:59:59.999 --> 99:59:59.999
Through this process we can 
improve our team.

99:59:59.999 --> 99:59:59.999
One person involved in the study 
said this:

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

99:59:59.999 --> 99:59:59.999
that drive a higher standard of coding.

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

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

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

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

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

99:59:59.999 --> 99:59:59.999
that we're seeing on any individual change.

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

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

99:59:59.999 --> 99:59:59.999
rather than trying to explain it to them

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

99:59:59.999 --> 99:59:59.999
about a particular change. We often talk
at abstractions

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

99:59:59.999 --> 99:59:59.999
about things. And those are really
comfortable conversations.

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

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

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

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

99:59:59.999 --> 99:59:59.999
the technical communication 
on our team

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

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

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

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

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

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

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

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

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

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

99:59:59.999 --> 99:59:59.999
And so why is that?

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

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

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

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

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

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

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

99:59:59.999 --> 99:59:59.999
this culture around code reviews.

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

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

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

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

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

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

99:59:59.999 --> 99:59:59.999
when you're providing feedback.

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

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

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

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

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

99:59:59.999 --> 99:59:59.999
Believe it or not.

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

99:59:59.999 --> 99:59:59.999
it's what you've changed.

99:59:59.999 --> 99:59:59.999
The context is why that's changed.

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

99:59:59.999 --> 99:59:59.999
So the context is 
what problem is it solving.

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

99:59:59.999 --> 99:59:59.999
chief impediments to a quality review.

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.

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

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

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.

99:59:59.999 --> 99:59:59.999
"Use type column first in
multi-column indexes"

99:59:59.999 --> 99:59:59.999
The specifics here aren't 
particularly important

99:59:59.999 --> 99:59:59.999
if you don't understand what this means.

99:59:59.999 --> 99:59:59.999
The problem here is that 
there is no why.

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

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

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.

99:59:59.999 --> 99:59:59.999
I can't really learn anything from it.

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

99:59:59.999 --> 99:59:59.999
get a thumbs up and move on.

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

99:59:59.999 --> 99:59:59.999
"Can you provide some more context here?"

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

99:59:59.999 --> 99:59:59.999
or adds a comment that says 
something like this.

99:59:59.999 --> 99:59:59.999
Right?

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

99:59:59.999 --> 99:59:59.999
That's not better.

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

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

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

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

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?

99:59:59.999 --> 99:59:59.999
Probably not, right?

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

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

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

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,

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

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

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

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

99:59:59.999 --> 99:59:59.999
That's what we do first.

99:59:59.999 --> 99:59:59.999
Why is it a problem?

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

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

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

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

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

99:59:59.999 --> 99:59:59.999
So that's a lot of context.

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

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

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

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

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

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

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

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

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

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

99:59:59.999 --> 99:59:59.999
and put them on equal footing with you.

99:59:59.999 --> 99:59:59.999
So I challenge you to provide
two paragraphs of context

99:59:59.999 --> 99:59:59.999
with every change you made.

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

99:59:59.999 --> 99:59:59.999
torturous to describe in two paragraphs.

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

99:59:59.999 --> 99:59:59.999
But how much more time?

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

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

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

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

99:59:59.999 --> 99:59:59.999
gets to live on in the commit.

99:59:59.999 --> 99:59:59.999
We're gonna squash that 
and we're gonna save that.

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

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

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.

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

99:59:59.999 --> 99:59:59.999
What about as a reviewer?

99:59:59.999 --> 99:59:59.999
What can we do to

99:59:59.999 --> 99:59:59.999
make it so that our 
feedback is well received?

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

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

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

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

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

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

99:59:59.999 --> 99:59:59.999
You're gonna perceive that one way.

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,

99:59:59.999 --> 99:59:59.999
it's going to be perceived 
much more negatively

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

99:59:59.999 --> 99:59:59.999
negativity bias in our
written communication.

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.

99:59:59.999 --> 99:59:59.999
One way to do that
is to offer compliments.

99:59:59.999 --> 99:59:59.999
I would suggest that you do that.

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

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

99:59:59.999 --> 99:59:59.999
those are great to call out.

99:59:59.999 --> 99:59:59.999
It lets everybody know that...

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

99:59:59.999 --> 99:59:59.999
Rather than just always nitpicking
at the change.

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