RailsConf 2015 - Implementing a Strong Code-Review Culture

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