By Hamzah Tariq on Mar 25, 2019 10:21:00 AM
We’ve interviewed Derek Prior, a Developer at Thoughtbot and host of The Bikeshed Podcast. We discuss how to build a code review culture, diving into the benefits of code reviews, the essential elements to make them effective and how to handle conflict if it arises.
Derek Prior is a developer for Thoughtbot in Boston. He co-hosts a web development podcast called, “The Bike Shed”. He speaks about development practices at conferences, including the talk, “Cultivating a Code Review Culture”. Derek, thank you so much for taking time to join us today. Do you have a bit to share about yourself?
I’ve been speaking at conferences and meet-ups lately about code reviews, like you said. It’s something that I’ve had a lot of experience with over the last 10 years or so.
Code Reviews Vs. Pair Programming
Some people mean different things when they use the term “code review”, like “pair programming”. What do you mean by code reviews?
There’s this term, and it’s not my term, called, “modern code review”, and that’s what I’m talking about. What that means… The definition of that is basically, asynchronous or tool-driven, generally like, lightweight reviews. Most of the people I work with that means a GitHub pull request. That’s typically what I’m talking about. Pair programming is great, and has a lot of the same benefits. But when I’m doing pair programming, I still like to have another person to review the code… I still like to have that other person, also review the code.
The reason for that is in pair programming, you’re kind of building up a solution with your pair, as you go. I really like to see at the end, if that holds up for another developer who might have to work on this next week kind of thing. Like, does this make sense to somebody who wasn’t there all along the way.
“It’s much more helpful to focus on the cultural benefits of code reviews”
Benefits of Code Reviews
Why are code reviews important?
Everybody here, when they hear about this, everybody’s natural reaction is to say that they catch bugs. That is true. If I have code reviewed that’s going to have less bugs, than code that isn’t reviewed. But I think that that puts too much importance on the “finding bugs” part. Microsoft actually did a study of this, Microsoft Research, in 2013. Where they found that people consistently said the number one benefit from code reviews is finding defects in code.
But then when they looked at actual data collected in their code review tools, and talked to people after they did code reviews, what they actually found was that people got way more benefit, way more cultural benefits, out of sharing knowledge with each other, or keeping up with what everybody’s doing, and knowing what’s going on in that other part of the code base. Finding a really interesting alternative solution to a problem that they hadn’t thought of, just by getting somebody else’s viewpoint.
Those are the much more important things. I feel like they’re much more interesting than finding defects. Finding defects is actually, frankly, really hard. A lot of times I’ll talk to people about code reviews, and they’ll say, “Well we did code reviews, but we still had all these bugs. They weren’t helping us catch the bugs”, and yeah, they’re not going to catch all the bugs. By saying, ‘The chief thing we get out of code reviews is “defects finding”’, what we’re really doing is setting ourselves up for those situations where people say, “But you did a code review on this, and there’s still a bug.” Right?
It’s hard to find all the bugs, because when you’re doing one of these lightweight code reviews, you’re really just looking at a slice of a change. You’re looking at the diff, and to know exactly how that’s really going to impact your system, you have to know the entire system. You can catch edge cases where, “Oh, you didn’t check to see if this was nil”, or whatever the case may be. Knowing exactly where this is going to screw up your data, is hard to know, without knowing the whole system. Code review is great for some defect tracking, or defect finding, but it’s not a panacea. Instead, I think it’s much more helpful to focus on the cultural benefits of code reviews.
How should you work code reviews into your workflow?
What we typically do at Thoughtbot is, when I finish up a PR, I will paste it into Slack, or whatever. We’ll paste that in and say, “Can I get a review for this please?” If it’s not urgent, that’s basically all that happens, right? If it’s urgent, I will ping somebody directly. Ping a couple people directly and say, “Hey, this is a production fix, can you take a look at this?” Generally, that’s enough, with the way we work, to get your code reviewed within the next few hours. Which is, generally sufficient.
You can move on to something else. You can review somebody else’s code in that time. You can kind of trade, if somebody else is coming up and finishing something up. You can be, “Oh yeah, I’ll take a look at that. You take a look at this one for me.” That kind of thing. We take a really, lightweight approach to it.
When you think about it, there’s a lot of natural breaks throughout your day that you can work these in to right? For me, I come in in the morning, “are there any PRs outstanding I can look at?” Then I get started. Then, right before lunch, or in the afternoon, if I’m going to take a coffee walk around Boston Common or something, I’ll look then. Either before or after. There’s plenty of times I find, that I can just work these in naturally. There’s no need to schedule them.
I’ve worked with teams that try to schedule them, or try to say, “This person is going to be the one, that’s going to be chiefly doing code reviews this week”, and I’ve never seen that work particularly well. I just try to say, “Keep it lightweight, friendly”, things like that. People are really surprised, or skeptical, to hear that this works. The biggest thing that makes this work is, keeping small, discrete, pull requests, that are going to be much easier reviewed, and providing some excellent context. That’s the “why” you’re making this change, not necessarily the “what”. Ultimately, the big secret to this is that, most of these code reviews only take me five minutes. It’s not a big commitment.
What to cover in a Code Review
What type of things should a code review cover?
It’s really important that each person involved in the code review, doesn’t operate from a central checklist, but rather just looks at things that interest them about a change right? Or interests you overall. Maybe you just got finished reading this great book on design patterns. You’re looking for a way to apply this one design pattern. That’s what you’re going to look for. Great. Fine. That’s valuable. You’re going to teach somebody something.
Or maybe you’re really interested in web security, and you’re going to take a look at things from that perspective. Or accessibility, you’re going to take a look at things from that perspective. That’s where having a good blend, on your team, of people who are interested in different things, really pays off, in code review. We’re all going to look at code reviews, and try and see if there’s an obvious bug. I said, not stressing the bug finding, is a big key to this.
But yes, you’re going to look for them, and you’re going to comment on them when you find them. What I really like when people do, is just sort of take their slant on how they think software development should be. Just kind of look at it with their lenses and see, “Oh, did you consider doing this alternative thing over here?” I typically look at … I’m always harping on naming. Right? Part of what I think makes code review so great, is that it’s a great place to have a technical discussion about your actual software. Rather than in the abstract, like when we go to conferences and things like that.
I harp on naming a lot to say, “Do these names give us a good basis for a conversation, about this? Are they descriptive?” That’s really important to me. I look for test coverage, to see … Like I said, I’m not hunting for bugs throughout the entire system … But I am looking to see, “Is this adequately covered, by test?” I’m not going to look at every single test, but I’m just going to to kind of get an idea of “What kind of tests would I expect to see in this change?” I’d expect to see a feature spec. Or I’d expect to see a unit test. That kind of thing. Just make sure that those are there. Stuff like that.
Like I said earlier, the biggest thing is just everybody kind of, bringing whatever it is that they’re interested in … Whatever they’re an expert at … To the table, so that we can all learn from each other.
“Having conflict in your code reviews is actually really beneficial”
Code Review Author and Reviewer Best Practice
As a code author, what should you do to get the most from a code review?
The number one thing is context. When you’re submitting a pull request, you’ve been working on this thing for four hours, or eight hours, or two days, or sometimes even a week, or whatever the case may be. You have a lot of context built up in your head. Some things seem really obvious to you at this point. But, to the person reviewing it, they weren’t there for you. They don’t have that context. What you really need to do is think about… It really helps if you’ve been making several small commits along the way, where you’re describing what was in your head.
But as you prepare the change… If you’re going to use a GitHub pull request, and you’re going to submit your pull request to GitHub, make sure you give a nice description of, a summary, of everything … What I really like to see is, not what … A lot of people will summarize what they did? That’s kind of important, the bigger the change gets, so I know what to expect when I’m looking through the change? But it’s not the only thing I want to see, right?
I can figure out what a change does by looking at the code. Unless it’s totally obfuscated. What’s really interesting is why. Why are we changing it? Why is this the best solution? What other solutions did you consider? What problems did you run into? Is there an area of the code you’re really unsure about, that you’d really like some extra eyes on? That kind of thing. Those are going to help you get a much better review, by setting up everybody else to be on the same page as you.
When you’re reviewing code, how do you do it without being overly critical or needlessly pedantic?
This happens a lot. Where code reviews that aren’t done particularly well, or overly critical, can kind of, lead to resentment among the team. The big thing there is, negativity bias, that you need to be aware of. If I’m talking to you … Like I am in this conversation, and I give you some technical feedback, and I say, “Oh, why didn’t you use this pattern here”, whatever. You’re going to perceive that in one way. But if I say that same exact thing written down, it comes off more harsh. You’re going to perceive that more negatively.
It’s much more subject to your particular mood. I can’t influence it with the way I say something. It’s just a fact that, the same feedback written, is going to be perceived more negatively. One excellent way to get around that, is to give feedback in a manner that’s more of a conversation. What I like to do is ask questions. At Thoughtbot, we call this “asking questions, rather than making demands”, right?
Instead of saying, “Extract the service object here, to reduce some of this duplication”, I would say, “Hey, what do you think about extracting a service object here, to eliminate this duplication?” Right? They’re very similar comments, but now I’m opening it up to a conversation, by asking you a question. Like, “Oh, what do you think …?” Sometimes I’ll even say something like, “What do you think about doing it this way?”, and I’ll provide a code sample. Then afterwards, if I don’t really feel particularly strongly about it, I’ll be like, “No, I’m not really sure though, what do you think?” Just to kind of say, “I can see how this would go either way.” “What do you think?”
Clarifying how strongly you feel about a piece of feedback, is also something that can help. Making sure that you’re addressing people’s feedback. That’s basically, how to avoid being overly critical. Is just asking nice, friendly questions, that open up a conversation? The pedantic part, I guess would be like, when you start arguing in circles. When you’re not coming to a good conclusion here, right? What I’d say there is just, make sure you’re focusing on high value things. Don’t go edge case hunting everywhere.
There’s a lot of times you can find edge cases that, just aren’t going to come up. Or, maybe they are and they’re important. But really, what you want to be focusing on is, the more higher value type stuff that we talked about earlier. Try and focus on those, to stay away from these like, pedantic arguments that you can into in code review.
“The big secret is that most of these code reviews only take me five minutes”
Handling Conflict in Code Reviews
Inevitably, with discussions between developers in code, disagreements are going to occur, from time to time. How should you handle this type of conflict?
I think the first thing to recognize, is that, having conflict in your code reviews like this, is actually really beneficial. Right? As long as they’re the right types of conflict and nobody feels bad about them afterwards, that kind of thing … Because if you’re agreeing with your teammates all the time, and nothing interesting is happening in your code reviews, you basically have a monoculture, and those are dangerous on their own right.
Like I eluded to before, you want everybody bringing their own experiences, and their own expertise, and sometimes those are going to clash with each other. My experience was, doing things this way, led to these problems, and your experience was, doing things this way, was actually really beneficial. We’re going to have conflict about that. The key is that, handling those conflicts properly, is how everybody on the team is going to learn.
What I suggest, when you find yourself going back and forth, a couple comments about something, and you don’t seem to be getting anywhere … Nobody’s yielding, right? What I try to do is take a step back and be like, “How strongly do I feel about this?” Do I feel like, “This is a quality issue, that if we go out with this code, like it is today, it’s going to be a serious problem immediately?” Or is this kind of like, “It’s not really the approach I would have taken, but, maybe it will work. Or it’s a reasonable solution.” That kind of thing right?
Often times, that’s actually what it is. Once we get beyond a certain level of quality, what we’re talking about is trade-offs, all the time. That’s basically all we’re talking about when we’re doing computer programming. Like I said, once we get past the initial quality is, the trade-offs involved. If you’re arguing about what is essentially a trade-off, maybe it’s time to be like, “Okay. You know what? I would prefer to do it this way, but I can see why you’re doing it that way, and why don’t we just revisit this, if we need to, when we have more information?”, right?
Sometimes, you’ll argue about code, for a while, and then it goes into production. Then you don’t touch it for six months. Which means, it was fine. Even if you didn’t think it was the greatest looking code in the world, like it was written, and it doesn’t have bugs that are obvious. It doesn’t need to be continually improved so, whatever, it’s fine.
If you can table the arguments you’re having until you have more information, and then kind of revisit them. Be like, “Oh, okay. I see now we have this bug, or we have this additional feature we need. I can see how that thing you said earlier, would be a better approach here.” That kind of thing. Waiting until you have more information to, really dig in again, is probably a good thing to do.
Can you recommend any resources for those wanting to improve the effectiveness of their code reviews?
We have guides. If you go to github.com/thoughtbot/guides, there’s a bunch of guides in there. Some of them are like style, and protocol and things like that. But there’s also a code review guide in there. Whether or not you agree with everything in there, I think it’s a good example of something that I’d like to see more teams do. Where they provide, “This is what we write down, and agree to.” Then have pull requests against, when we want to change. What we think code reviews should be. How we think they should operate.
Every piece of feedback, should it be addressed? Yes, I think so. At least to explain, “Okay, I see what you said there, but I’m going to go in this direction instead.” That kind of thing. I’d like see more people take a look at that, and then adopt it for their team. Other things that can really help with code reviews, are style guides. Which we also have in that repo but, there’s also many community style guides. Just avoiding those arguments about, “Oh, I’d really like see you call …” Like in Ruby, we have map and collect under the same method, right?
Let’s be consistent. Let’s pick map, or let’s pick collect. Or something like, just writing that down somewhere, making that decision once. Rather than arguing about it on all pull requests, those types of things. That’s basically it. I’d like to see more people, get on board with and kind of documenting their experiences and, their expectations out of these things.
Derek, thanks so much for taking time and joining us today.
Yeah, thanks for having me. It was good fun.
Its extremely important to learn how to build a code review culture. Code reviews are great for code quality as they reduce errors, help deliver better code and encourage teams be more compassionate towards one another.