Ask HN: How do you keep code reviews from taking forever? | Hacker News Reader

Ask HN: How do you keep code reviews from taking forever?

15 pointsteeray27 comments

Code review is important, but I find it's often a momentum-killer. A few things can happen after you open a PR:

* Nobody reviews it for days because everyone is busy working on their own stuff. This is likely because code review isn't as rewarded as pushing code ("I implemented X" sounds better in a standup than "I reviewed a bunch of PRs yesterday").

* You end up in an endless back and forth with one or more reviewers (e.g. push-review-fix-push-review-fix-push-review...). If this occurs over the course of days, the "fix" portion becomes an interruption where you have to remember the context of the changes, make the changes, then submit them.

These are the two instances I can think of off the top of my head, but if you have other examples and strategies for combating them, I'd love to hear them.

View on HN

You get vastly better results if your team has culture of having smaller pull requests. The effort required to review a PR is non-linear, so make sure that:

- high-level tests can be submitted before code implementing them exists, they just need to be disabled and then selectively re-enabled in subsequent PRs - this way any logical design issues should surface in tests which should be much cheaper to deliver and iterate on than implementation

- any refactoring is submitted separately (it's easy to verify because it doesn't affect any valuable tests, so it leaves out a whole class of issues), even if it didn't happen "organically" during development - take some of the burden on you so that your reviewers don't have to waste time trying to logically unlink these things in their head - it should be much easier for you, you wrote that code

- features are delivered in an incremental fashion whenever it makes sense - you probably don't need to submit a fully working solution, you can prop it up it with fakes or simply not expose the changes to the user (feature flags, or just simply not hooking up a feature yet)

If you do that, it makes things much more digestible, at some cost of understanding of the overall solution (starting with tests and necessary refactoring should help here describe what you want to achieve) and burden from having to split things. It has worked very well for me, in multiple companies.

Small pull requests is the key. https://www.microsoft.com/en-us/research/wp-content/uploads/... was a study written on it (among other things.) This lines up with traditional code review wisdom.

> "I reviewed a bunch of PRs yesterday" sounds bad in standup

The real problem is here. Code review must be a first class citizen of your team culture to be successful. It is the responsibility of management/scrum master/etc to foster and if necessary enforce the culture

Usually comes down to one of

  * I don't see any divide by zero bugs. Approved.
  * Some inane argument about code style that the linter enforces.
  * I spent 8 hours figuring out what you did, why you did it, reviewing how code matches the requirements, running the code, testing all the scenarios.
  * Can we rewrite this entire thing because I prefer a different pattern?

haha, I've definitely seen some of those. Comment standards are worth obeying, personally I like the code review pyramid https://www.morling.dev/blog/the-code-review-pyramid/

Code review must be a first class citizen of your team culture to be successful.

Yup, you nailed it. Overall it sounds like the culprit here is inexperienced management -- that somehow "knows" that code review is important, but doesn't (really) understand why. To the extent of being able to justify the, you know, time cost involved to upper management constantly breathing down their necks to get the next shiny new button working.

As someone that spends a lot of my work week reviewing code and documentation, here are some initial thoughts that come to mind:

1. break large feature branches into smaller PRs so they are more easily digestible by reviewers. If this is not feasible, at the very least, ensure each commit in the PR is concise and meaningful, so reviews can look at the PR commit-by-commit if needed.

2. have specific reviewers or "owners," for different areas of the codebase. Tossing a PR into the abyss without specific reviewers isn't a winning strategy for getting the desired feedback in a timely manner.

3. do not backtrack when requesting PR changes, meaning, each review should only review the code that has been implemented since the last review (don't retroactively ask for changes introduced in the original PR if it's the second or third time reviewing).

4. Code reviews should be celebrated at the team level; they shouldn't be seen as a net negative on one's velocity, rather, they're a net positive for the team's velocity.

5. Be proactive: be an active reviewer of your peers code and they'll feel a bit more urgency in assisting with your PR reviews as well.

My 2c,

Some dev on the internet who spends a lot of time reviewing code

In my experience, code reviews are currently viewed as time sinks because of the cognitive load they put on the reviewer. I agree with the other comments here about a team culture that values code reviews as an important process. IMO, the tools surrounding code reviews can be improved. In particular, any time spent trying to make changes smaller and easier to follow is a big win. We use and develop DiffLens (https://github.com/marketplace/difflens) for this. As a team, it's made reviewing our changes easier and lets us probe deeper into the changeset. If your code base is mostly TS, JS and/or CSS, feel free to try out DiffLens! We're around to answer any questions or take feedback :)

> Nobody reviews it for days because everyone is busy working on their own stuff.

Just ask people nicely. If they do your review promptly and objectively, ask again.

> You end up in an endless back and forth with one or more reviewers (e.g. push-review-fix-push-review-fix-push-review...).

The fault is clearly theirs in this case. They're being obsessive and not showing respect for your time. In any case, if it's one person doing this, you can ask them to pre-review your solution with them before you start implementing it.

We had to admit the obvious fact that code review had become a useless ritual for us.

- people asking each other to "rubber-stamp" one-liner PRs

- breaking up changes into small PRs holding up progress, because a reviewer requesting a change on an earlier chunk would need to get propagated to all later chunks

- no one wanting to touch large PRs

Our solution was drastic: we abolished code review.

- Big changes, for some definition of "big," go through an initial design review in a Google doc.

- Each feature is owned by exactly one person. You merge code directly into main at whatever pace you choose. This removes the bottleneck of CR.

- All bugs that pertain to your feature come directly to you and you work on them as your first priority. This encourages us to write correct, appropriate, well-tested code.

- If a feature owner leaves the team, the feature is reassigned to someone else. In the rare case when the feature is too hard to hand off, we rip it out and rewrite it. It's obvious that the support burden of such a feature would have been too high. This encourages us to write clear, simple, understandable code.

We're pretty happy with this arrangement so far.

One of the benefits of a waterfall development model over these agile strategies is that code review becomes a whole-team, front-and-center piece of the process, not a distraction from other tasks.

With each revision of our product spec, we work first to write or modify code to meet the new spec and then, once everybody has tested and merged all their changes, we pause new development to review and verify the code. We tend to end up each working on different modules, so we can maintain complete independence in review by each reviewing others' modules with minimal attention to who authored each individual diff. We just keep a spreadsheet tracking all the different files, their authors, and whether they've changed since the last review; at the start of a new review, we assign a reviewer to each file and then the author and reviewer work the file through review-resolve rounds until there are no further findings.

Since everybody is fully focused on the review, it goes by quickly; since there's no expectation of review during development, we can develop quickly without holdups.

1) Management, or at least team-lead buy-in to how important it is to get code reviews done and prioritized appropriately.

2) Code review checklist and a coding standard document. Any review item that's not part of the checklist or a violation of the standard is a "suggestion" and can be ignored. That by itself cuts down the back and forth like you wouldn't believe.

Someone brought it up in a retro. We then kept the shorter ones under 24 hours, though a huge one may still take a few days. I think you just have to set it up as a policy with colleagues, and most would agree that it's reasonable.

The back and forth is part of the process, but it's a lot shorter once someone has actually read the code.

Also lowering the bar helps. We do review badly formatted stuff and misleading variable names. And definitely check for major bugs. But not poorly named variables and inconsistent architecture, unless it conflicts with a written code style doc.

Part of the delay is thinking of a way to "negotiate" a style change. I'd recommend putting a written style guide to minimize that. If you want it to be camel case or whatever, negotiate it on the style guide.

Let me add here something that was maybe not mentioned clearly in the comments: Time is an important ingredient of good code reviews.

If the team members have their capacity allocated fully then there is no time to do proper code review. Thus code review is ignored as much as possible or it is seen as time lost from the project. Will also transfom the code review into an action that will be superficial.

Before fixing everything else you must first understand if the team has time to do code review. That means that the expectation is not that they should produce the same output but also do code reviews. If this is the case then the team does not have time for Code Review.

And only after there is time for this, you should go into fixing/improving the process.

you and your team must consider merging to master (or whatever your production branch is called) the important thing, not pushing in isolation, though this only works if review blocks merging to that branch

this way, it stops being "I reviewed PRs" and becomes "we [the team] merged N commits to master"

in my previous team, I got into the habit of doing code review first thing in the morning, and right before the end of day, so others could either start by checking my review comments, or merge at or near the beginning of the day

it helps if commits are very small

Stick managers on the code reviewer for holding up the sprint/release. Like, ask for status updates hourly on them.

If there's no obvious reward for code reviewing as opposed to implementation, create one.

Ours don’t. Precommit formatters and linters do the drudge work. Tests check the logic. Critiquing the design takes ten mins. Gitlab shows the context.

Do in the morning or end of day, if in the zone.

I mean - this assumes the person is solving the correct problem and that everything is easily testable or being tested correctly.

I’ve reviewed many PRs where they were trying to add a feature or solve a bug but were not achieving the goal. It passed everything by whatever standards but only reading the code made you realize - it wasn’t going to give you the result you desired.

Then you send them back to the drawing board, presumably with better information this time. If this is happening regularly you need better training, etc. If they still can't handle it, let them go.

Not an unexpected issue in my experience, the price of doing business.

This is so true. The hardest bugs and usually the ones that can get caught with a good code review are misunderstandings from the developer or business analysts not making the expected behavior clear.

Tests don't catch that. Unless the tests are written by someone other than the developer, which honestly I haven't seen.

As a contrived example if a developer interprets that they need to multiply instead of add, their method and tests might work 100% but they will give the wrong result. Even worse, in certain cases 2 * 2 = 2 + 2 so they will give the right result with the wrong calculation.

Steve McConnell covered these common failure modes in his book series, which I recommend if your team is encountering them.

Paraphrasing, "you should be endeavoring to make unique mistakes…"

If a CR is taking to long, you have a meeting about it, talking through the solution. Or if you are not happy with the solution, you’d need to implement it yourself and argue for it. Key here is that changes needs to be smallish. Anything not small needs to be discussed beforehand, and agreed upon, before starting. It’s ok to stop development and call for a meeting if the solution gets too big.

Here’s an idea to speed up PRs - don’t do code reviews on every PR.

Gate your PRs with automated checks for anything you really want to enforce (docs, formatting, tests, etc)… but when it comes to everything else, leave that to dedicated code review sessions where the feedback will be less rushed, more thoughtful and far more likely to be taken constructively.

Works a charm for my team - mostly infra/terraform and python.

Measure the time it takes. Post metrics (median time, oustanding prs, how long they've been there) on a dashboard on a monitor where everyone can see. Try to get the time down using all the suggestions here.

wait, i have a 55MB pull request encompassing 12,872 files. And it’s all good.

> Nobody reviews it

It does take some culture shifts to make reviews a normal part of the culture. Send reviews to specific people and expect deadlines. (Sometimes this is assigning "code owners" for specific areas. Sometimes this is "have a review buddy".) Add bots and/or calendar reminders if you need to poke someone to get the review done. If "nobody" is the right person to review it, don't have a review at all. It's fine to say that changes to certain areas or below a certain size don't need another reviewer if they pass builds/tests. I've seen teams with bots that auto-merge small PRs after a specified amount of time with no review comments. I've heard of teams that set "these types of PRs are fine to self-merge if" checklists for applications with active QA teams where it's fine to merge code quickly for quick feedback and iterate in further PRs.

> You end up in an endless back and forth with one or more reviewers (e.g. push-review-fix-push-review-fix-push-review...). If this occurs over the course of days, the "fix" portion becomes an interruption where you have to remember the context of the changes, make the changes, then submit them.

First, automate anything that can be automated. Use linters. I prefer the sorts of opinionated linters/formatters at this point that auto-format rather than just complain (prettier for the web stack is a strong example). Once automated remove "style" problems as something to ever mention in a PR once the automated checks have passed.

As a review submitter, find ways to ask where the balances between "cosmetic", "suggestion", "tech debt", and "show stopper" are. Eliminate cosmetic comments (again, if it matters configure it in a linter and automate it away). Work on the confidence to ignore suggestions/tech debt (learn from them, but don't feel compelled to answer every single one with a response or a commit). One key way to to both "ignore suggestions/tech debt" and find that balance between what is actually a "show stopper" is variations on the very simple question "Can we move this to the backlog for a follow up PR?" or "Can I keep this in mind for the next PR for this feature?" A lot of things that sound like show stoppers are merely suggestions, but in the context of a PR it can sometimes feel like everything is important that the reviewer mentions.

As a reviewer, find better ways to set those expectations up front. Mark suggestions as suggestions. Sometimes just adding "Suggestion:" in front does so much to set expectations on how important a note/comment is. Use more phrases like "if you have time" or "something to keep in mind for next time". It's too easy to just note every single thing "wrong" you find and forget to mention that most of what you found "wrong" doesn't matter in short term. I find it is also useful to be proactive sometimes: if it is a tech debt solution that can wait, merge the PR and add the tech debt item to the backlog mentioning the PR without "making a full round" of it.

Some of that is setting expectations for when a PR is "done". It's okay to expect PRs to be "done" quickly and to instead expect follow up PRs. Putting too much pressure on "a PR needs to be perfect" gets in the way of the ideals of continuous integration. You want code interacting sooner rather than later. You can fix up the suggestions and tech debt next time, but it's sometimes important to have that extra time for integration problems to be discovered and QA testing to happen on the "not quite perfect" PRs.

I will tell you what I do, your mileage might vary.

I try to keep my code changes small. I rather have 4 changes in four different PRs, than a single one. Have you ever opened up a PR, saw 25-100 files changed and you just moved on instead of going through it?

Also, I will add a small explanation of what I'm trying to solve, and if I did something "smart" I'll explain why.

Finally I keep a small list of people that tend to approve my changes fast and ping them if enough time has passed.

On the flip side when I see a PR, I will let people know that I'm looking at it OR that I will look at it at a certain time and they should ping me if they don't hear from me (approval or comments). Also I have compiled a checklist that I follow for my PRs which speeds up the process, at least for me.

My PR checklist has three sections, what I should evaluate, how I should communicate and how to write things in the PR itself. It's an accumulation from things I have seen in HN, the Google Code Review process, and reminders to myself. I tend to sound more authoritarian than I mean to be, so it's a reminder to be more gentle.

   # Checklist
   1. Design.
   1. Functionality - Does the code do what it's supposed to do?
   1. Complexity - Are individual lines, functions, classes too complex (aka hard to understand quickly).
   1. Maintainability - too tightly coupled? Configurations hard-coded.
   1. Comments - are comments actually necessary, they should explain why. Are there any comments/TODOs that can now be removed?
   1. Documentation - has the appropriate documentation been updated
   1. Good Things - comment on the good things as well
   1. In general, favor approving a CR once it's in a state where it definitely improves the overall code health of the system being    worked on, even if it's not perfect

   ## Communication
   * Accept that many programming decisions are opinions.
   * Ask good questions, don't make demands (what do you think about naming this `user_id`?).
   * Good questions avoid judgement and avoid assumptions about the author's perspective.
   * Ask for clarification ("I didn't understand. Can you clarify?").
   * Avoid selective ownership of code (mine, not mine, yours).
   * Avoid using terms that could be seen as referring to personal traits (dumb, stupid). Assume everyone is intelligent and well-meaning.
   * Be explicit. People don't always understand your intentions online.
   * Be humble (I'm not sure - let's look it up).
   * Don't use hyperbole (always, never, endlessly, nothing).
     * Don't use sarcasm.
   * Keep it real. Be yourself.
   * Talk synchronously. Post a follow-up comment summarizing the discussion.

   ## Reviewing Code Communication
   * Communicate which ideas you feel strongly about and those you don't. (Nitpick: )
   * Identify ways to simplify the code while still solving the problem.
   * If discussions turn too philosophical or academic, move the discussion offline to a regular Friday afternoon technique discussion. In the meantime, let the author make the final decision on alternative implementations.
   * Offer alternative implementations, but assume the author already considered them. ("What do you think about using a custom validator here?")
   * Seek to understand the author's perspective.
   * Sign off on the pull request with a  or "Ready to merge" comment.
   * Remember that you are here to provide feedback, not to be a gatekeeper.

Reply on news.ycombinator.com