Ask HN: How do you review code?

We've tried scheduling 2-3 times per day where everyone should focus on PR review. It's quite easy to get lost in one's own tasks. This helps maintain the understanding that everyone needs to work together to move all features/bugs/chores/etc. along.

We also have a slack channel for pull requests, where everyone subscribed gets notified when someone posts a PR for review, or later for re-review.

I think timing and communication can be an issue on any team, especially with distributed resources (I'm remote). We regroup regularly as a team to discuss process, issues, and anything else on our minds. We experiment with solutions, and try again and again until something sticks. It's not always perfect the first time, but we end up with something that works for us. For now, we try to hold each other accountable to our scheduled times, and ping (and re-ping) on slack for reviews.

Our team has a similar bottleneck. A few weeks ago I attempted to solve the problem during the static showdown hackathon, but I haven't touched the project since the hackathon ended so it's pretty rough:https://ss16-obelus.firebaseapp.com

The thing we wanted was a news feed of sorts of outstanding PRs so we could track who checked them off and which ones needed our attention first.

I guess it wasn't a big enough pain point to continue developing on it :(

Also I think a whole web app is a clunky tool for the job, something like a chrome extension might be more appropriate.

We have a slack channel for dev and people issue review requests in it. This works best when most people can review the code obviously. There's also an informal culture of trading (you promptly review my branch; I'll promptly review yours.)

We cut some corners and assume devs are ok to address minor complaints w/o further review. So you see "approved pending minor issues X, Y" which doesn't require further review.

We do something akin to pair programming for reviews, though we don't pair program at all. If you can, you sit with the reviewer and discuss. This often leads to a joint effort to fix anything the review brings up and speeds turnaround.

Finally, it helps if you get serious about a linter and, as much as possible, software enforced code style.

Like others have said, pre-commit hooks for linters are very helpful. Also, CI is a must.

These alone can significantly speed up code review since you can be fairly confident you won't break anything new. Then, you just have to focus on: "does this fix the issue that was found?"

Then, it's just a matter of breaking up issues into meaningful, modular tasks that can be taken care of without days of code review.

We use phabricator which has a much clearer flow for blocking on code reviews and you can easily see your list of diffs awaiting your review. I find the workflow much better than GitHub. That said, communication is really key, and that comes at an organizational and cultural level to define what's an appropriate amount of nagging you can do to get reviews through.

At a previous company, we adopted a system similar to Google's early review process. Every repo would have two owners (for redudancy in case of sickness). We'd put the emails of both owners in an OWNERS file. Then, whenever a pull request would pop up, an automated process would send out an email to the owners. It would continue badgering them, decreasing the period between reminder emails in proportion to how long the pull request has stood outstanding.

This was a great, easy process that helped ensure a timely review for our pull requests.

We had a similar problem and have made a lot of progress by reducing the size of changes submitted for review. We used to submit code only when we thought a feature was complete, and the median review was probably 1000 lines. Now we're closer to 100-200.

This has a ton of positive effects:

  - Reviewing code is much less daunting
  - Reviewers give more detailed feedback
  - Developers get feedback on lower layers (such as model definition) before writing a ton of code that depends on it
  - Reviews come quickly enough that developers don't need to switch to another project before continuing

It was a bit difficult to make this transition. We started out by explaining what we wanted to do (submit shorter diffs) and why, and people generally were on board. But most people didn't know how to do it. It's not as simple as submitting a diff whenever you write 100 lines of code, since the diffs need to be coherent. Here are some techniques we used:

  - Use feature flags to commit unfinished features to master/trunk without exposing them
  - Submit model changes as their own diffs, then API changes, then UI changes
  - As you work on a feature, pick out parts that seem ready for review, stage and commit them, send for review, then branch again from that point and continue working.  This takes a decent amount of git-fu, so train folks accordingly.

Another thing to note is that our reviews are not at all evenly distributed around the team. We have a team of 13 engineers, and it's probably an 80-20 split—80% of reviews are done by 20% of reviewers

  Please manually add newlines to your blockquotes,
  otherwise your comments are near-unreadable on
  touch-screen devices. And they'll be much better
  on mouse-driven ones, too.

We had exactly the same problem, so we built a bot for our Engineering room in HipChat that pesters the entire room about how many pull requests are currently open, what they are, and links to them, once every 2 hours (during business hours).

It's worked really well. We've also extended that same bot to do other things like remind you about support requests, etc.

1. Read the requirements/expectations document

2. Read how much of the code is reused

https://en.wikipedia.org/wiki/Pareto_principle

You're right that the problem is cultural at its root -- there's no one solution to "making" programmers eat their vegetables. But it helps to have many automated reminders, so that the nagging doesn't cause people to hate a single member of the team: when a reviewer is added or a review is updated, your tool should remind the reviewer(s) on a regular basis that new work is pending.

I've been working on a better review tool for teams who have built their process around GitHub pull requests (https://www.omniref.com/code_review), and one thing I've learned from talking with teams is that GitHub's notification system is just comically broken for a team review workflow -- completely overwhelming with emails, but in the wrong way (i.e. a notification for every edit). People just ignore it. So perhaps you should check out a different tool?

+1 to the other comments about having linting and CI to reduce the nitpicks and basic verification in PRs.

One technique I've used and liked is to have the PR author create a calendar invite for the review, usually with enough lead time to be reasonable, eg the following afternoon. This isn't an old-fashioned "everyone get in a conference room" code review -- you still it do it individually at your machine -- but it allocates the time.

The author puts one or two people that they would like to review as required, and the rest of the team as optional. That helps the reviewers carve out time to get it done, encourages people to RSVP as either reviewing or not, and makes for a well-understood deadline when to get your comments in by if you want to have any influence on the PR.

It's not a perfect system, but might be worth giving a try if you are having problems getting prompt sign-up for reviews.

This is one of the things I found most difficult/interesting as an intern.

It's really important to have the local, pre-code-review infrastructure to be able to keep developing and testing in the same direction while a code review is pending. I had very good experiences with Phabricator, which has support for "dependent diffs" so you can create further separate diffs (sort of like PRs) that build on as-yet-unreviewed diffs to avoid requests ballooning in size while awaiting review.

We pester each other incessantly on the team chat to actually do the review. We do cross-functional teams, so there's little dilution of responsibility - only 1 or 2 people are clearly qualified to review your code, and they generally do so in a timely manner.

We use a standard GitHub process with a standard branching model (master and feature branches) for reviews. We require that each change is a separate PR and unless it's an emergency we require that each change is reviewed before committing. We tried dedicated tools for reviews but found them too much of an overhead.

Unlike some places with strict procedural requirements (e.g. Google with it's OWNERS and readability process) we depend on an honour system which given our size (~30 devs) seems to work rather well. We assume that doing code reviews is an important - if not the most important - thing in a dev's life so we spare no effort in order for the code to be reviewed properly while also not blocking others' work.

However, we heavily rely on tools to ensure that human reviewers spend their time on things automated review can't figure out. These are generally linters and code smells detectors (e.g. reek in Ruby) which run on CI (Codeship). We use these helper tools to push statuses to GitHub and we block merging branches on CI passing so if tests are failing - which might even be just for a stylistic issue - human code review process would not even start. We also use a hosted code quality analyser (codebeat - which is our own product) to track quality over time, keep everyone on the same page and help devs and reviewers focus on the right things.

Overall what we found out is that code review is a learning process and very thorough reviews for new devs help them ramp up very quickly so that subsequent PRs require less and less attention.

We had a similar problem with lag for code reviews, so I wrote a bot to nag the Hipchat (moving to Slack this week) channel and remind people that repos had pending PRs that needed review. This helped quite a bit of getting code reviewed.

Keeping PRs smaller definitely helped as well and now we typically have 3-5 PRs per day go in.

Reviewing is switching gears as well. When you're coding you can't just jump into a code review mode for a moment. It's not a culture issue, this is IMHO inevitable for any work environment that requires deep concentration and immersion into the work inside your head.

So you'll want to batch the reviews somehow, based on your schedule. On the other hand, as a programmer, you don't want to be blocked by code reviews. So you'll want to finish a somewhat self-contained set of changes, push them out for a review, and continue on top of those locally. If the reviews suggest new modifications, you'll do those and rebase your local new work on top of those. It doesn't matter that much how the changes are reviewed as long as it's not blocking work. There should be an emergency vent for the cases when unreviewed changes do block significant other changes.

Ask someone to do a review? Why would you ask, other than out of politeness with the clear expectation that the answer "no" should come with a solid business reason? It's their job. If someone isn't doing their job, then management has either not made it clear to them what their job is, or management needs to ask them why they aren't doing their job. To my mind, it sounds like what you need is for management to get their act together, sort out the review process and then enforce it.

I worked somewhere where a reviewer was assigned before code was cut. Once the code was ready for review, the reviewer became the publicly visible person responsible for that particular feature/bugfix/whatever.

People who had reviews sitting on their plate for too long would find themselves being asked questions by management about why they weren't doing their job and why they were holding things up [1]. We wouldn't put up with someone getting round to doing the implementation whenever they felt like it; likewise, we wouldn't put up with someone getting round to doing the review whenever they felt like it. As it was, this never happened, because it's part of the job.

[1] There are, of course, good and bad answers to this question, much as there would be good and bad answers to the same question asked of the implementer rather than the reviewer. The answer "Oh, I just haven't got round to it, I'll get to it eventually" is as acceptable as you probably imagine.

I find that reviewing code is a pain. Not just for me, but for the submitter too if I take too long / I am excessively nitpicky about code style issues. I don't think good reviewing tools exist right now. You can rope together multiple tools which require constant babysitting, but there's no good low maintenance all-in-one solution (or if there is, please suggest it!)

Anyway, given that caveat, here are my tips:

(1) Have a "checkpatch" script that can be run over the code before it is sent. This script should do all the nitpicky coding style stuff. If the submitter didn't check their patch, you immediately reject it (some projects automate this). Here is an example from the qemu project: http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/checkpatch....

(2) Have something like patchwork, or github pull requests, or openstack gerrit reviews, which tracks which submitters have submitted patches, which is the latest version of their patches, and so on.

(3) Have some automation which runs the test suite against submitted patches and immediately rejects any which fail.

(4) Have documentation which tells people how to submit patches, what scripts to run, and encourages submitters to split their changes into reviewable commits. This is more art than science, but good coders will eventually get it. Here are qemu's instructions: http://wiki.qemu.org/Contribute/SubmitAPatch

Now when you review a patch:

(5) Apply it to your tree and make sure it compiles. Enable warn=error so no warnings can escape your notice.

(6) If it comes from a trusted submitter, and looks sane, I generally ACK those without a detailed review.

(7) If an earlier version of the patch was sent, go back and look at your comments, then check everything that you asked to be fixed was fixed.

(8) Go through each commit, line by line, and check everything. This is the tedious bit I'm afraid. But hopefully the previous steps will have rejected the worst mistakes.

(9) Try to be level headed and provide constructive criticism (even if the patch is objectively terrible). Also try to turn around patches quickly (I'm very guilty of not doing this, so do as I say not as I do).

We use Jira to track development tasks, and have two states for review: Awaiting Review and Under Review. Every branch (and thus every PR) has a corresponding dev task. We configure Jira so it lights up the Review column in red when there are too many items in it. It's a fairly unsubtle hint that items in the column need to get reviewed before moving on to the next dev task.

We also use Stash (now Bitbucket) rather than Github, so we can configure it to trigger Jira state transitions.

Depending on how features get factored, you can end up requiring certain tasks as pre-requisites before other tasks can get started. Then there may be direct communication / nagging involved. But we try and cut things up a little bit differently, so that there's less scope for lag to impact overall feature delivery time. The main idea is to prevent PR reviews from being blockers for multiple people.

We used to sit down together to the do review. The reviewer set an open timeslot for a possible review, the first who grabs it sits down and together they review it. The calendar entry helps to timebox the review process, the verbal communication speeds it up and helps to avoid conflict.

One more addition, take care of the important bits only, don't fight over coding style or such.

We had a similar problem - a lot of pull requests being stuck in a back-and-forth remarks and fixes. The "social" solution was to increase the amount of communication in the team: instead of everyone having their own tasks and working in a silo, we made multiple people (ideally, the whole team) responsible for a task, splitting work as we go. The technical solution was to use pair programming instead, at least for some changes. Both worked really well.

Pair programming achieves a similar result to code reviews (another person knows the code very well, and is able to criticize it and improve on the design). It also does it much better because the review is done as the change is being made, and not after the fact, which improves the understanding of the reviewer and allows them to give immediate feedback.

The old code review process still works as a fallback, and is especially good for small but tricky changes: I need to do something novel, it's not a big change but there are a lot of ways to do it, can you comment on my approach.

One other thing that we also ended up doing is "ping-pong pull requests": if you give me a pull request, I don't just criticize it leaving the issues for you to fix, I fix them and leave that for you to review. Then you can do the same, etc. As a result, both people get better acquainted with the code, instead of just one of them being the "owner" of a change.

We use Jira, GitLab and Slack in our agile teams at work. I've written some automation that listens to GitLab webhooks and (a) announces new Merge Requests in the #team-review channel, which all team members are in, and (b) creates sub-tasks on the original Jira story or bug, which appears on our Jira boards.

But yeah, most of it is about setting expectations. I think our conclusion was mostly that review times under about ~24h are okay, so within that space people can organize their own time.

That said, I have been trying to get my team members to create smaller commits. I'm personally convinced that review time is exponential in commit size, such that reviewing a branch with 20 small commits is much more efficient than one squashed commit. That said, this hinges on actually doing the work to create good commits: no fixup commits, one logical change per commit, etc.

This has come up enough that I think I should write a blog post about it some time.

Our code review policy is to require a minimum of 2 people who did not contribute to your changeset review and approve your code prior to merging. We've been able to reduce the amount of time a review takes in four big ways:

(1) Automated style checker. By adding a pre-commit hook to validate code style in an automated way, the amount of noise in PRs went down dramatically.

(2) Don't do architecture review in PRs. If you're going to make architectural changes, do so in a lightweight design document and get approval there first.

(3) If it languishes, go and physically (or virtually) ask the specific people to take a look. This works best in my experience when you find someone else who's having trouble getting their code reviewed and offering a trade. This person is then likely to review your code quickly in the future if you do the same.

(4) Feature flags allow you to get incomplete but atomic and correct units of work merged to master without showing them to your end-user.

This way, code reviews are not about style (much), not about architecture (much) and simply about making sure you executed on the plan correctly. Turn-around times went from days to hours in many cases.

    grunt eslint
    npm test

We use Gerrit. Gerrit is a review system derived from the Git technology.

From the Git user's point of view, Gerrit is installed as a git remote. When you develop some changes, you push them to the Gerrit remote, using a special branch naming convention. Gerrit dynamically takes this push and converts it to a review item, which is then subject to a web-UI-with-email-notifications-based review workflow.

A special "Change-ID:" item in the commit header identifies the change. This lets you push revised versions of the commit: if the Change-ID matches, Gerrit recognizes a push as being the new version of a previous push. These versions are organized in the review process under one item. You can see what the differences are between different versions and so on.

The review process involves multiple users in reviewer roles. They get notified via an e-mail that there is something for review. Then they can look at the changes, and approve or reject the commit. General comments can be added to the item and comments can be attached to individual lines of code, also, so you can easily say very specific things about specific pieces of code.

We have automatic reviewers who participate also. For instance, if a kernel change is proposed, then "checkpatch" is run on it automatically, and posts an approval or disapproval to the Gerrit item. The code also has to build for several targets and pass automatic smoke tests.

Commits are ultimately merged by users who have the power to do that. They take into account all the other votes and then cherry pick the change.

Gerrit is far from perfect, but it provides a decent workflow, and is a huge improvement over ad hoc development. For instance, the Change-ID can be a pain. It's assigned by Gerrit when you first push something, and then you must remember to add that same Change-ID to the new version of the commit. I wrote myself a script which generates a Change-ID; I stick it into the very first commit, so Gerrit doesn't have to make one up for me.

To use Gerrit, users have to acquire a somewhat better than average understanding of git; it's just a little bit more advanced than the "pull, commit and forget" use cases whereby you pretend that git is SVN or CVS. Things should be set up to prevent common mistakes; for instance, lock the repository from direct pushes, so only pushes to Gerrit are allowed. I would make a blank "Change-ID:" part of the commit message template, and give users a way to generate these things, to avoid the common mistake of pushing a new version of a commit without a Change-ID, causing Gerrit to generate a whole new review item.

We use ReviewBoard, which allows you to tag both a group and specific members of the team with review requests.

I generally tag more than one person, and if possible people who know that bit of the code. I wait until I've had two signoffs (with verbal badgering if people are being slow; we are all near each other in an open plan.)

I will often find myself holding out for the one person who "owns" that bit of the code before pushing the patch, though.

One suggestion that we tried: People agree to do check for assigned reviews first thing, and as they return from lunch. This way people aren't interrupted from their development flow by the reviews.

We have three rules which help here.

1. You cannot start a new task before all reviews have been done 2. You need to talk to the author somehow when reviewing. 3 If you've pair programmed you don't need to review.

The first rule makes people allocate time. The second rule takes focus off minutia and moves it more to the issue or imrovement and suggested changes, including architecture.

The last rule is there since we believe that reviewing is a natural part of the cooperation when pair-programming.