Ask HN: Does your team do code review? How? Why not?
1 comments
I've worked with several companies and the successful ones do code review somehow. Before I go into them, let me preface with we have always required 100% code test coverage. If even one test fails, it doesn't go out. We have also used Github everywhere I've worked. The primary flow is that a developer writes code, submits a PR, the code gets reviewed, then it gets merged and deployed.
That said, there were two major strategies that we used:
Strategy 1: Gatekeeper
The project lead heads up the code reviews. She is the gatekeeper that determines whether code is good enough. For minor bug fixes and small changes, one extra set of eyes is generally sufficient. For larger changes and new features, she often recruits one or more developers to assist with the code review. All code review takes place in Github, with face-to-face meetings if there are any nuanced discussions that need to happen.
The major upside to this is that the lead knows what the big picture is. She knows if Joe has worked on this feature and should be consulted or not. She knows that Sarah is very familiar with this technology and should be consulted. She knows that this feature can't go out until that feature is. She knows that this bugfix might affect Henry's feature. This means that she can properly coordinate releases throughout the team and make sure everyone's on track.
The major downside is that there's a single point of failure. If the project lead isn't thorough, bad shit can slip through. Doesn't happen often, in my experience, but enough that there should have been more failsafes.
Strategy 2: More Eyeballs
Nobody "heads up" code review. Each pull request requires two people to approve it. Any number of people can comment and review it, but the minute the second person says "Looks good to me" or "+1" or whatever, then the PR is allowed to be merged.
The major upside to this is that you get more eyeballs on the project. You'll have a couple of different perspectives, so there's a greater chance that everything has been thought of.
The major downsides to this are: no birds-eye view, choosing the reviewers, and coordinating fixes/features. A big hurdle to this is choosing the reviewers. If you have 6 people on the team, and 2 people need to approve it, who do you pick? How do you pick? Randomly? Round-robin? Imagine that you know that Joe is a lax reviewer and Bob is a strict reviewer, and since you're tired of working on this feature, you give it to Joe so that it will go out sooner with less hassle. Big problem.
Furthermore, imagine that Sarah pushes a PR to a feature that was previously built by Mike, but she selects Joe and Henry as reviewers. Even though Mike worked heavily on this feature in the past and is probably the expert on it, it is entirely possible with this system that the feature could make it into production without Mike even being aware of it.
----
Those are the two strategies I've worked with. It's probably clear which one I prefer. That said, Strategy 2 has a lot to recommend to it. If your team is small enough, it's probably a good idea. For larger teams, I recommend either Strategy 1 (gatekeeper) or mixing the two (gatekeeper + 1-2 other approvals).
That said, there were two major strategies that we used:
Strategy 1: Gatekeeper
The project lead heads up the code reviews. She is the gatekeeper that determines whether code is good enough. For minor bug fixes and small changes, one extra set of eyes is generally sufficient. For larger changes and new features, she often recruits one or more developers to assist with the code review. All code review takes place in Github, with face-to-face meetings if there are any nuanced discussions that need to happen.
The major upside to this is that the lead knows what the big picture is. She knows if Joe has worked on this feature and should be consulted or not. She knows that Sarah is very familiar with this technology and should be consulted. She knows that this feature can't go out until that feature is. She knows that this bugfix might affect Henry's feature. This means that she can properly coordinate releases throughout the team and make sure everyone's on track.
The major downside is that there's a single point of failure. If the project lead isn't thorough, bad shit can slip through. Doesn't happen often, in my experience, but enough that there should have been more failsafes.
Strategy 2: More Eyeballs
Nobody "heads up" code review. Each pull request requires two people to approve it. Any number of people can comment and review it, but the minute the second person says "Looks good to me" or "+1" or whatever, then the PR is allowed to be merged.
The major upside to this is that you get more eyeballs on the project. You'll have a couple of different perspectives, so there's a greater chance that everything has been thought of.
The major downsides to this are: no birds-eye view, choosing the reviewers, and coordinating fixes/features. A big hurdle to this is choosing the reviewers. If you have 6 people on the team, and 2 people need to approve it, who do you pick? How do you pick? Randomly? Round-robin? Imagine that you know that Joe is a lax reviewer and Bob is a strict reviewer, and since you're tired of working on this feature, you give it to Joe so that it will go out sooner with less hassle. Big problem.
Furthermore, imagine that Sarah pushes a PR to a feature that was previously built by Mike, but she selects Joe and Henry as reviewers. Even though Mike worked heavily on this feature in the past and is probably the expert on it, it is entirely possible with this system that the feature could make it into production without Mike even being aware of it.
----
Those are the two strategies I've worked with. It's probably clear which one I prefer. That said, Strategy 2 has a lot to recommend to it. If your team is small enough, it's probably a good idea. For larger teams, I recommend either Strategy 1 (gatekeeper) or mixing the two (gatekeeper + 1-2 other approvals).
Thanks for the detail! My personal experience is generally in strategy 1 as you outlined, especially in being that lead person. I know a lot of people like the appeal of strategy 2 (myself included) but like you mentioned, there's a number of ways that critical code can fall through the cracks. It's great for a general "good work, I like the way you did that" or "what about doing it like this" kind of review but beyond that doesn't provide many safeguards. That being said, I've heard a number of complaints about the organizational/cultural aspect of strategy 1.
In full disclosure, part of the reason I'm asking is to do some research for a product of mine: https://pullapprove.com. Currently I feel as though it serves as a fairly flexible platform that can accommodate either (or both) of the strategies in various ways, and just want to make sure things are headed in the right direction. As you mentioned, the process for choosing reviewers could be handled several different ways and I wonder if there's room for some tools to assist in that. An interesting one to me (which PullApprove doesn't yet support) is automatically choosing reviewers based on git blame information (https://github.com/facebook/mention-bot) -- which could get at your last point.
I feel like there's a ton of value in doing code review that, I personally, drastically underestimated for a long time. I'm interested to hear more about the kinds of problems different groups have with code review and trying to figure out if/where/how a tool like PullApprove can alleviate some of the pain and make it an easier process to adopt.
In full disclosure, part of the reason I'm asking is to do some research for a product of mine: https://pullapprove.com. Currently I feel as though it serves as a fairly flexible platform that can accommodate either (or both) of the strategies in various ways, and just want to make sure things are headed in the right direction. As you mentioned, the process for choosing reviewers could be handled several different ways and I wonder if there's room for some tools to assist in that. An interesting one to me (which PullApprove doesn't yet support) is automatically choosing reviewers based on git blame information (https://github.com/facebook/mention-bot) -- which could get at your last point.
I feel like there's a ton of value in doing code review that, I personally, drastically underestimated for a long time. I'm interested to hear more about the kinds of problems different groups have with code review and trying to figure out if/where/how a tool like PullApprove can alleviate some of the pain and make it an easier process to adopt.
Cool tool! I run a GitHub code review app myself (https://reviewable.io) and have a similar feature built-in to customize the completion condition: https://github.com/Reviewable/Reviewable/wiki/FAQ#can-i-cust.... The main difference is that you took a declarative approach, which I contemplated but discarded after checking with my users and discovering how much variety there was in their approval algorithms. I'm running user scripts on AWS Lambda instead: http://blog.reviewable.io/your-code-my-app-no-worries.
I'll be curious to see how your project works out!
I'll be curious to see how your project works out!
Awesome! I actually contemplated going that route as well, but obviously decided not to. Cool use for Lambda though. Do you find that people are using that feature?
Some people are, but mostly just a hardcore few who are using protected branches on GitHub. I expect usage to tick up as I ramp up (in app) marketing for the feature and move upmarket into larger companies with more formal development workflows. But also, this was an investment in infrastructure that will allow me to offer other deep customizations later on.
Does your team actually do some form of code review? If not, why not? If so, how do you do it? Face to face? GitHub? Other tools or services?