00:00:12.160
Good afternoon, RailsConf! I hope you're all feeling great after lunch. Thanks so much for coming; I'm 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.
00:00:23.080
We'll discuss doing them well and what it means for the culture of your team when you're the type of place that does them well. So, let's start with a show of hands just so I can get an idea of where everybody's at. How many of you conduct code reviews as a regular part of your day every day?
00:00:40.440
Okay, and how many of you really enjoy doing them? A few less. And how many of you do them because you feel like you have to? It's about equal. All the people who said they really enjoy them also said they do them because they have to.
00:01:01.719
So why is it that we do reviews in the first place? This is pretty easy, right? It's to catch bugs. We have somebody look at every individual line of code, and we aim to find what's wrong with it. Not really. That's not why it's interesting.
00:01:14.320
I've been doing code reviews for over ten years now, and I used to hate them—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.
00:01:38.079
Yes, we're going to have fewer bugs in code that's been peer-reviewed than in code that has not been peer-reviewed, but studies 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.
00:01:57.560
We think that by doing code review, we're getting this much quality assurance when in reality, we're getting somewhere down here. So, why is that? The reason is when we do code reviews, we're looking at a slice of a change.
00:02:10.080
We're essentially looking at the Git diff, and we can catch syntax issues or problems like calling a method on nil, but we can't catch the really serious issues that happen when the whole system interacts and corrupts your data.
00:02:34.280
Code review is good for some level of bug catching, but it's not the end-all be-all. So what are they good for? I already told you that I think code reviews make me better every day, and I want you all to feel the same.
00:02:43.560
Well, in 2013, Microsoft and the University of Lugano in Switzerland came out with a study on the expectations, outcomes, and challenges of modern code review. They looked at various teams across Microsoft, a huge organization with several teams and developers of different experience levels all working on different products.
00:03:01.600
They surveyed people to find out what they got out of code review, what they liked, and what they didn’t like. Once they finished surveying, they watched these individuals do code reviews, asked them questions afterward, and then manually classified all the feedback. They found that people consistently ranked finding bugs as a very high reason for doing code reviews.
00:03:24.960
However, it turned out that the chief benefits were actually 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.
00:03:43.560
Through this process, we can improve our team. One person involved in the study said that code review is the discipline of explaining your code to your peers. That drives a higher standard of coding. I think the process is even more important than the result.
00:04:02.440
By going through the process of code review the right way, we're going to be improving our team, regardless of the actual results we see on any individual change. I also like the definition that code review is the discipline of discussing your code with your peers.
00:04:11.440
Code review is one of the few chances we get to have a straightforward conversation about a particular change. Often, we talk in abstractions—we come to conferences like this and discuss large abstractions, which are comfortable conversations.
00:04:26.720
In code review, we have to get down to the implementation details and discuss how we’re going to apply these things. Like much else that we do, it's a communication issue. If we get better at improving code reviews, then we're really improving the technical communication of our team.
00:04:43.680
The study also found that the 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.
00:05:00.080
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. As I mentioned earlier, I've been doing code reviews for over ten years now, but only in the last two to three years have I started to see a real improvement in what I’m getting out of them and in myself.
00:05:20.760
I think it's because I'm part of a strong code review culture at Thoughtbot. At Thoughtbot, we often go to clients of various sizes to help them with issues they have.
00:05:36.960
Nobody ever comes to us and says, 'I really need help improving the code review in my team.' However, when we get on the ground with those teams, we often find that there isn’t a strong code review culture.
00:05:53.680
One of the challenges is how to get people to embrace this culture around code reviews. We suggest following a lot of little rules in our guides, but they can really be boiled down to two key rules of engagement. Two things you can do today to start getting better at code reviews with your team.
00:06:09.880
The first is something you can do as an author, and the second is something you can do as a reviewer when you're preparing and providing feedback. First, as an author, what are we going to do to make sure our code reviews get off on the right foot?
00:06:33.680
This quote here is from Gary Vaynerchuk; he was talking about social media marketing or something not interesting, but it's also applicable to code reviews, believe it or not. 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.
00:06:56.560
Your code doesn’t exist for its own benefit; it solves a problem. The context is about the problem it's solving. The study I cited earlier found that insufficient context is one of the chief impediments to a quality review.
00:07:12.680
So, as an author, we need to be aware of this and get out in front of it by providing excellent context around our changes. Let’s look at a real-world example. This is a commit or pull request title that you might see come across your email: 'Use type column first in multicolumn indexes.' The specifics here aren't particularly important, but the problem is that there's no 'why.'
00:07:38.520
The change affects the order of columns in multicolumn indexes, but it’s not clear if that's the best solution to the problem you're solving. I can't really learn anything from it, so it's not interesting for me to review. I would just thumbs-up it and move on.
00:07:54.960
In such cases, I would comment and ask for more context. A lot of times, what we find happens is that the submitter will update the pull request or add a comment providing some additional context.
00:08:06.320
They might post a link to GitHub or their issue tracker, which isn't enough. Many people provide a short explanation and say, 'For more detail, see this ticket.' But what you're doing there is making the reviewer hunt for this context.
00:08:25.760
When they click the link, they might see a report of a problem, maybe some back-and-forth discussion about how to solve it, and then a link to the pull request. They must hunt through it all and put it together to tell you whether or not it's the right solution.
00:08:41.520
Here's an improvement: instead of just saying, 'Use type column first in multicolumn indexes,' provide context. Identify the problem with index ordering, explain why it is a problem, and back it up with some PostgreSQL docs that link to more information if needed.
00:09:00.000
Also, because this particular change involves Rails, we need to be concerned with multiple adapters, so back it up with some MySQL documentation. Finally, explain why this is the best solution. That's a lot of context.
00:09:13.440
What's important to note is that now, as I review this, I know why this change is being made, and maybe I've even learned something about how multicolumn indexes work by reading the supporting documentation. There's value in reviewing this.
00:09:29.840
As authors, we need to provide sufficient context. We’ve worked on this change for hours, so it's crucial to bring the reviewer up to speed and put them on equal footing with us.
00:09:42.480
I challenge you to provide two paragraphs of context with every change you make. At first, it’s going to be really painful, and some changes will be torturous to describe with two paragraphs.
00:09:54.960
Yes, this will take more time—possibly five extra minutes—but it avoids a round of questioning about why you're making this change. The extra bonus is that the context we saw earlier gets to live on in the commit history.
00:10:06.240
Our Git history will maintain that crucial information, whereas a fleeting comment in an issue tracker will disappear when we stop paying that bill. So, that's what we can do as an author. Now, what about as a reviewer?
00:10:19.440
What can we do to ensure our feedback is well received? I like to call this "ask, don't tell." To start, it's important to note that research shows there's a negativity bias in written communication.
00:10:34.440
If I have a conversation with you face-to-face and offer feedback, you’ll perceive it one way. If I provide that same feedback via written communication, such as a pull request review, it tends to be perceived more negatively.
00:10:54.560
That's why we must overcome this negativity bias to have our feedback taken the right way. One way to do that is by offering compliments.
00:11:03.520
If you find something in a pull request that taught you something or is done particularly well, call it out! It lets everyone know, 'Yes, you taught me something; that's great! Thank you very much!'
00:11:20.760
But there will come times when you need to provide critical feedback, and the best way to do that is by asking questions rather than making demands. I’ve noticed that when I take the time to ask questions, the technical discussions are more satisfying for everyone involved.
00:11:34.320
For example, consider a comment like, 'Extract a service to reduce some duplication.' That’s a command, and there’s no dialogue. The original author of this change might feel they have to either do it or engage in what seems like an argument—or they could just ignore it.
00:11:54.480
Ignoring is likely the worst option. An improvement might be to say, 'What do you think about extracting a service to reduce some duplication?' This simple phrasing opens up the conversation.
00:12:08.480
One likely outcome is, 'Yes, you’re right! I can reduce some duplication by extracting this service. Thanks a lot!' And now the reviewer feels good because they provided valuable feedback that was well received.
00:12:27.160
If the author disagrees with the change, they were just asked for their opinion anyway, so they feel free to express themselves. What we’re facilitating here is technical discussion.
00:12:38.760
Code reviews can be a means for us to have excellent discussions on technical matters. It’s beneficial to use conversation openers like 'Did you consider…?' or 'Can you clarify…?,' which are far better than 'What the hell's going on here?'
00:12:50.080
These types of approaches soften suggestions and avoid negativity, thereby leading to better discussions. Moreover, they're excellent techniques for providing feedback.
00:13:06.080
If you’re a junior member of a team and someone more senior submits a change, it’s natural to feel tentative about providing feedback. But framing your input as a question is a great way to provoke engagement rather than mere compliance.
00:13:22.520
After opening with these conversation starters, you can then segue into practical suggestions. By doing this, you help everyone get on the same page.
00:13:36.000
Similarly, senior members can provide feedback to juniors in this way. If I issue a command, the apprentice may complete the task without understanding why, resulting in a missed learning opportunity.
00:13:49.440
Unlike straightforward commands, questions encourage dialogue and create a learning moment. Remember to use positive and engaging language. Avoid phrases like, 'Why didn’t you just…?' that can come off as judgmental.
00:14:07.040
Such wording implies the solution is simple and builds an unnecessary pressure on the receiver. Instead, ask open-ended questions to facilitate a constructive conversation.
00:14:21.680
For instance, instead of asking 'Why didn’t you do this?' you can say, 'Why didn’t you consider calling map here?' This ownership of suggesting alternatives allows for discussions rather than disagreements.
00:14:36.240
When in the role of a reviewer, we have an opportunity to soften our suggestions and avoid negativity. This leads to better discussions and greater collaboration.
00:14:52.320
This approach can also benefit junior developers. If they feel hesitant providing feedback as they review changes by seniors, they can make inquiries and express their want to learn, steering the discussion positively.
00:15:06.960
This format works well when more senior developers provide feedback to juniors as well. Instead of outright commands, we engage them in the discussion.
00:15:20.360
We can also use specific phrases and question formats to facilitate technical discussion around code changes instead of reverting to commands.
00:15:32.601
By using this inquiry-based approach, we keep the lines of communication open and lower barriers to giving and receiving feedback.
00:15:52.878
This also applies equally well for situations in pairing or during real-time discussions—it's about fostering an inclusive dialogue.
00:16:10.480
There are definitely plans to use more queries in the form of conversation starters, and if you have suggestions to enhance that practice, I'd be glad to discuss it.
00:16:29.440
Now, how do you handle a situation with a gatekeeper—a sole person who does the reviews and you must get through them to get code merged?
00:16:42.760
In this case, I would suggest working with your co-workers to change that culture or find a new job. I’m serious; the benefits of good code review culture, as I’ve shared, are real.
00:17:03.640
So how do you provide context on refactorings, especially when there aren't new features to review?
00:17:12.200
My advice is to explain why you're doing the refactor. Remember, we’re not serving as QA in our reviews; we rely heavily on tests.
00:17:30.560
If your tests pass, I—as a reviewer—am interested in you describing the solution. Tell me why you felt the pain and what this refactoring resolves.
00:17:49.560
When it comes to QA and code reviews, do the code review first in case of significant changes. This helps to make QA's job easier, allowing them to focus on the overall picture.
00:18:05.840
Asynchronous reviews are perfectly valid. We've all had the experience of handling time zone differences; it's tough but manageable if there are others in the same zone.
00:18:19.640
We need to maintain balance, understanding that a wide-spanning team may have its challenges. However, as developers, we can forge good practices regardless.
00:18:33.440
Now, regarding defining criteria for reviews, you’ll often get asked what to focus on if it’s not catching bugs. Everybody should bring their own checklist, and that's how we enhance one another's expertise.
00:18:49.200
For me, timing is key. I prefer timely reviews and focus on prompt feedback for small changes rather than overwhelming each other with extensive reviews.
00:19:06.080
I prioritize the single responsibility principle as part of my review checklist, ensuring every object in the system has just one job.
00:19:20.120
Naming is fundamental; a clean name makes discussing code simpler and improves communication overall. Also, I focus on complexity—areas of the code that exhibit complex shapes may require further dives.
00:19:35.360
Test coverage naturally comes into play; I'll see what’s available for unit tests, controller specs, or feature specs covering the changes.
00:19:51.440
While I’m not doing QA, it’s essential to gauge test coverage during a review. Each reviewer contributes expertise, and I would expect the same from you when asked for feedback.
00:20:05.680
Style is important, too; a stylish codebase offers an impression of a unified team. A neat codebase is akin to a clean kitchen; all the parts are in their proper places.
00:20:22.560
But when people receive style comments, it often results in a negative association where the reviewer is seen as nitpicking rather than beneficial.
00:20:40.760
To counter that, adopting a style guide creates clarity; document the committee's standards, and all debates around style should occur in that avenue.
00:20:53.680
Using tools like RuboCop or HoundCI can help automate these reviews and remove the emotional aspect of organic discussions.
00:21:10.560
These automated systems will address style while preserving the focus on technical discussions. If we start doing these things right, we’re well on our way to cultivating a strong code review culture.
00:21:24.800
A strong code review culture transcends the quality of a single change; it strikes at the core of your team's dynamic. What you'll see is better code as a result.
00:21:42.280
It won’t just be about catching bugs; the code will naturally improve as discussions elevate solutions. I can't tell you how many times I thought I’d submitted my best work, only for feedback to enhance it even further.
00:21:58.440
In our discussions, we will collectively achieve better quality, better developers, a shared ownership in code, and a healthy culture of debate on a team.
00:22:17.960
This sounds like an excellent place to work! You’re going to be better every day, and those benefits of strong code review cultures are truly rewarding.
00:22:38.800
Now let’s have some questions. How do you handle someone who just isn't engaged in the process?
00:22:48.720
I think helping them see the value is critical. Make sure they know that when they give review comments, they're engaged. Be excited about their contributions! Even if I don’t necessarily agree, I try to make a few concessions to get them back on board.
00:23:09.080
Writing expectations down helps. If you plainly state that you expect team members to be involved in reviews, you create a framework around it. Many developers often claim not to have time for reviews, but I think that's a misconception.
00:23:30.080
When you finish a feature, you should always have time to look through reviews quickly. Encouraging junior developers to participate in the review process is just as vital.
00:23:51.680
When you ask them to review code from more senior developers, have conversations about their feedback openly. Acknowledging their perspective can lead to valuable discussions.
00:24:05.800
Reviewing code is like asynchronous pairing; it requires genuine dialogue. It’s crucial to foster conversations centered on learning and sharing insights.
00:24:21.760
What about scenarios where someone serves as a gatekeeper, acting as the sole reviewer of code? My advice would be to try changing that culture.
00:24:34.400
Or find another job that acknowledges the value of distributed collaboration in code reviews!
00:24:48.200
In cases of refactoring without new features, communicate the rationale behind your refactor. Remind reviewers they should focus on the bigger picture as well as maintain tests throughout this process.
00:25:00.440
Timing is critical; it’s helpful to conduct code reviews before QA, making sure changes are significant without compromising quality.
00:25:15.760
When dealing with time zones, consider that it may be hard for individuals to provide timely feedback. Prioritizing small, digestible changes can foster smoother, distributed feedback.
00:25:31.560
Expectations may differ across time zones; when working within these constraints, it is crucial to be open to alternative arrangements.
00:25:46.080
When asked what to focus on during reviews, it's vital everyone contributes their checklist so that the whole team can benefit from individual experiences.
00:26:02.640
For my part, I focus on the single responsibility principle, aiming to ensure every object in the system solely serves one purpose.
00:26:16.600
The importance of naming and clear communication cannot be overstated, as these aspects play a crucial role in cohesive discussions. Complexity can trigger issues in reviews, so I also observe areas that show intricate changes.
00:26:29.040
Test coverage is part of my checklist; I ensure that there are specs covering the features and edge cases. Remember, our role isn’t about conducting QA.
00:26:42.080
When beginning the conversation about style, the process of adopting a style guide can help create uniformity that everyone can rally around.
00:26:56.440
Automating checking of style standards through tools can help to mitigate unnecessary commentary while allowing discussions to focus on substance.
00:27:12.080
With these strategies in hand, we will begin establishing a flourishing code review culture. A strong code review culture leads to visibly improved coding practices.
00:27:21.760
This means a more cohesive team, ownership of code, healthy debates, and ultimately producing better quality code as a whole.
00:27:37.640
To wrap up the presentation, I encourage all of you to ask questions. Together, let's foster a positive atmosphere for improving our practices and driving collective progress!
00:27:54.280
Thank you for your attention, and I’m looking forward to discussing this more with you during the Q&A session!
00:28:46.920
...
