Good afternoon RailsConf.
I hope you're all doing great after lunch.
Thanks so much for coming.
I've very excited to be here.
My name is Derek Prior.
I'm a developer with thoughtbot...
...and I'm here today to talk to you
about code reviews, doing them well...
...and what it means for the culture
of your team when you are in the
type of place that does them well.
So, let's start with a show of hands.
Just so I get an idea where everyone is at.
How many of you are doing code reviews
as a regular part of your day,
every day already?
Ok. And how many of you really
enjoy doing them?
Okay, a few less.
And how many of you do them because
you feel like you have to?
It's about equal. Okay.
All the people who said they really
do enjoyed them also said
they do them because they have to.
Okay.
So, why is it that we do reviews in the first place?
This is pretty easy, right?
To catch bugs.
We're going to have somebody look
at every individual line of code
and we're going to find what's wrong.
Not really.
That's not why it is interesting, right?
I've been doing code reviews for
over ten years now.
I used to hate them.
And, they were the worst
part of my day.
We did it just for compliance
documentation at one of my former jobs
But now I think that code reviews are
one of the primary ways
that I get better at my job
every single day.
So
Yes, we're gonna have fewer bugs
in code that's been peer reviewed.
than in code that
has not been peer reviewed.
But studies on this show that
the level of QA that we get
out of code review, doesn't meet
the level of expectation that we have
as developers and managers.
So, that is, we think that by doing
code reviews we're getting this much QA
when in reality we're getting
somewhere down here.
So, why is that?
Well, the reason is
when we do code reviews,
we're looking at a slice of a change.
We're looking at the git diff essentially.
And we can catch syntactical issues,
or problems where you might be
calling a method on nil.
But we can't catch the really
heinous stuff which happens when
your whole system interacts
and corrupts your data.
So, code reviews are good for
some level of bug catching
but it's not the end-all be-all, right?
So, what are they good for?
I already told you that I think
that code reviews make me better every day,
and I want you all to feel the same way
Well, in 2013, Microsoft and the
University of Lugano in Switzerland
came out with this study: "Expectations,
Outcomes, and Challenges of Modern
Code Review." So, in it, what they did was
look at various teams across Microsoft
which is a huge organization - they have
several teams with different
uh, they have Sr. developers,
Jr. developers, managers, everybody
all working on a different product.
They surveyed all these people to ask them
"What is it you get out of code review?
"What do you like about it, what do you
don't like about it?"
When they were done surveying them,
they watched them do code review
and asked them questions afterwards.
And, finally, they looked at all of the
feedback that was given.
Every comment that was logged in their
code review system.
And they manually classified it
They said "This one has to do with a bug
that they found. This one has to do with
a style comment. This one has to do with
a possible alternative solution."
So after doing all this work,
what they found was that people
consistently ranked finding bugs very high
as a reason for doing code reviews.
But in the end, it was actually a lot less
about finding bugs than anyone thought.
The chief benefit they saw from code
review were Knowledge Transfer,
Increased Team Awareness, and
Finding Alternative Solutions to problems
That sounds a lot more interesting to me
than hunting through code looking for bugs.
Through this process we can
improve our team.
One person involved in the study
said this:
"Code review is the discipline of
explaining your code to your peers
that drive a higher standard of coding.
I think the process is even more important
than the result."
I really do like that last part
even though it's not on the slide, right?
The process is more important
than the result. And we're gonna talk
about that today. Just by going through
the process of code review the right way,
we're going to be improving our team.
Regardless of the actual results
that we're seeing on any individual change.
But I also like the definition here
"the discipline of explaining your code
to your peers." I tweak
it just a little bit to say
rather than trying to explain it to them
Code review is one of the few chances we
get to have a black-and-white conversation
about a particular change. We often talk
at abstractions
like when we come to conferences
like this, we talk in large abstractions
about things. And those are really
comfortable conversations.
In code review, we have to get down
to the implementation details
and talk about how we're
going to apply these things.
So, like so much else that we do,
it's a communication issue.
If we get better improving code reviews,
then what we're really doing is improving
the technical communication
on our team
So, the study also found that
those benefits I cited earlier
were particularly true for teams that
had a strong code review culture.
So, what does it mean to have a
strong code review culture?
To me, it means that your entire team
has to embrace the process.
Everybody has to be involved. It can't be
something that the senior developers
do for the junior developers.
It's a discussion.
That's what we're after here.
So, as I mentioned earlier,
I've been doing code reviews
for over ten years now. But only in the
last 2-3 have I started to see a real
improvement in what I'm getting out of
them, and in myself because of them
And so why is that?
I think it's because I'm part of a strong,
code review culture at thoughtbot.
And, at thoughtbot we often go to clients
of various sizes to help them with issues
that they're having. And nobody comes to
us and says, "I really need help improving
the code review in my team."
That never happens.
But when we get on the ground
with those teams, we often find
that there isn't a strong
code review culture.
One of the challenges that we have
is how do we get people to have
this culture around code reviews.
And there are a lot of little rules that
we suggest following in our guides.
But if you look at them, you can
really boil them down
to two key rules of engagement,
two things that you can do today
to start getting better at code
reviews with your team.
The first of them is something
that you can do as an author.
The second is something
that you can do as a reviewer
when you're providing feedback.
So first, as an author,
what are we going to do
to make sure that our code reviews
get off on the right foot?
So, this quote here is from Gary
Vaynerchuck and he was talking
about social media marketing,
or something, not interesting.
But, it's also applicable to code reviews.
Believe it or not.
So, in a code review, your content
is the code, it's the diff
it's what you've changed.
The context is why that's changed.
Your code doesn't exist for its
own benefit. It solves a problem.
So the context is
what problem is it solving.
That study I cited earlier found that
insufficient context is one of the
chief impediments to a quality review.
So, as an author, we need to get, we need
to know this and get out in front of it.
We're gonna provide excellent context
around our changes.
So let's have a look
at a real world example.
This is a commit, or a pull request title,
that you might see come across your email.
"Use type column first in
multi-column indexes"
The specifics here aren't
particularly important
if you don't understand what this means.
The problem here is that
there is no why.
So, I can look at this change
and I can say "Well, yeah, this changes
the order of the indexes, or the order
of columns on multi-column indexes
But I can't tell you if that was the best
solution to the problem you're solving.
I can't really learn anything from it.
So, this is a loss. It's not interesting
for me to review, and it's just going to
get a thumbs up and move on.
Actually, what I would do with a change
like this is comment and say,
"Can you provide some more context here?"
And a lot of times, what we find happens,
is somebody updates the pull request
or adds a comment that says
something like this.
Right?
So, okay, so I guess you guys
think that's not better, which is true.
That's not better.
So, this is probably a link to Github
or maybe it's a link to Geer or whatever
your issue tracker is, right?
And a lot of people do this.
They'll provide a short explanation and
say, "For more details, see this ticket."
But what you're doing there is you're
making the reviewer hunt for this context.
If they click this issue, are they gonna
see a good description of what you're doing?
Probably not, right?
They're gonna see a report of a problem,
maybe some discussion back and forth
about how to solve it, and then maybe
a link to this pull request or something.
But they've got to go through all this
and they've got to hunt for that context
and put it together until they are in the
right frame of mind to review the changes,
to tell you whether or not
it is the right solution.
So, here's an improvement.
It's not important to read this, again.
So, what we're doing is identifying
the problem with index ordering.
That's what we do first.
Why is it a problem?
We're gonna back it up with some
postgres docs, and those link off to more
information if you need it.
And because this particular change
was a change to Rails, and we need
to be concerned with multiple adapters,
we're also going to back it up with some
MySQL documentation.
And finally, we're gonna talk about
why this is the best solution.
So that's a lot of context.
But it's important to note that now, as
somebody is coming along to review this,
I know why this change is being made,
and maybe I'd even learn something about
how multi-column indexes work, by reading
through some of this documentation.
So there's value for me to review this.
So, as an author, we're going to
provide sufficient context.
What we're trying to do here, like
You've been working on this change for
four hours, two days, however long
it took you to fix this, right?
What you need to do is
bring the reviewer up to speed.
Let them know what you learned
in this process,
and put them on equal footing with you.
So I challenge you to provide
two paragraphs of context
with every change you made.
At first it's going to be really painful,
and there are some changes that it's
torturous to describe in two paragraphs.
And yes, it's going to take you more time.
But how much more time?
I don't know, like, five minutes, maybe?
And it avoids a whole round of,
that round of questioning I described
before, like "Why are you making this
change?" So we've headed that off.
And the extra bonus is that all of that
context we saw earlier
gets to live on in the commit.
We're gonna squash that
and we're gonna save that.
So rather than that one we saw before
that had an issue - a link to G or a link
to Github, that's going to go away
as soon as we stop paying that bill.
But our git history is going to stay with
us, and we're going to see that there.
Ok, so that's what we can do
as an author.
What about as a reviewer?
What can we do to
make it so that our
feedback is well received?
We like to call this
ask, don't tell.
To start off with, it's
important to note that
research has shown there's a negativity
bias in written communication.
That is to say if I have a conversation
with you face-to-face and I give you
some feedback, or maybe
even over the phone,
just in a place where you can hear
the tone of my voice, hear the inflection.
You're gonna perceive that one way.
If I give you that same feedback written,
like in the form of a pull request review,
it's going to be perceived
much more negatively
So, it's important that
we're cognizant of this
negativity bias in our
written communication.
We have to overcome this if we want
our feedback to be taken in the right way.
One way to do that
is to offer compliments.
I would suggest that you do that.
So, if you find something in a
pull request that is something you learned
or is something that you think
is done particularly well,
those are great to call out.
It lets everybody know that...
"Yes, you taught me something.
That's great. Thank you very much."
Rather than just always nitpicking
at the change.
But, there's gonna come a time when
you need to provide critical feedback