
Ask HN: Does your team do code review? How? Why not? - davegaeddert
I&#x27;m looking to learn a little more about how different development teams do code review.<p>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?
======
Jemaclus
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).

~~~
davegaeddert
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](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](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.

~~~
piotrkaminski
Cool tool! I run a GitHub code review app myself
([https://reviewable.io](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...](https://github.com/Reviewable/Reviewable/wiki/FAQ#can-i-customize-
what-makes-a-review-complete). 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](http://blog.reviewable.io/your-code-my-app-no-worries).

I'll be curious to see how your project works out!

~~~
davegaeddert
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?

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

