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