Implementing a Strong Code-Review Culture

RailsConf 2015

by Derek Prior

The video, titled "Implementing a Strong Code-Review Culture" by Derek Prior, focuses on the evolving purpose of code reviews and the importance of fostering a positive code-review culture within development teams.

Main Points Discussed:

- Purpose of Code Reviews:

- Traditional views see code reviews as a means to catch bugs, but this is a limited perspective.

- Derek emphasizes that modern code reviews should focus instead on socialization, learning, and teaching.

Benefits Observed from Research:
- A study by Microsoft and the University of Lugano highlighted that while bug detection remains a high expectation, the key benefits of code reviews include knowledge transfer, increased awareness, and collaborative problem-solving.
Creating a Strong Review Culture:
- A synergistic approach is necessary where all team members participate, rather than having senior developers critique junior ones alone.
- The focus should be on communication and discussion about code, rather than merely pointing out issues.
Author Responsibilities:
- Authors of code should provide clear context around their changes, as insufficient context can impede quality reviews.
- An effective strategy includes offering detailed explanations of problems being solved and the rationale for chosen solutions.
Reviewer Responsibilities:
- Feedback should be framed as questions rather than commands to promote dialogue and inclusiveness.
- Initiating reviews with positive remarks helps in reducing the negativity bias typical in written communication.
Best Practices for Reviews:
- Focus on principles such as single responsibility, readable naming, and test coverage during reviews.
- Establishing a style guide and automated style-checking tools can alleviate emotional disputes over coding style.
Cultural Considerations:
- Derek emphasizes that a strong code-review culture enhances team dynamics, collaboration, and overall code quality, fostering an environment of shared learning and ownership.

Conclusions:

Derek Prior encourages teams to prioritize open communication and ask clarifying questions as a foundation for effective code reviews. Establishing a healthy code-review environment not only elevates coding standards but also nurtures developers' growth and fosters productive technical discussions. By implementing these strategies, development teams can cultivate a strong code-review culture that positively impacts overall productivity and collaboration.

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 ...