[dcraw]

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

[simonhfrost]

I agree it's important to reduce code review sizes, but I try to avoid reviews that end up leaving incomplete code in production such as having individual domain changes and feature flags. If a feature is cancelled before it's completed there will be unused code checked-in - which to me is technical debt.

I prefer to split domain changes into different commits on the pull request so they can be inspected individually - eg #1 Create API, #2 Consume API with model, #3 View Logic. And for feature flags, try to figure out a possible fully implemented subset of features - eg show new model properties to the user that will in the future be editable.

[tamana]

Feature flags are huge win for building reliable systems. You want them. Incomplete implementations should be modular, like complete implementations. Don't fool yourself, all code is tech debt. If you aren't writing modular PRs, you are in for a world of pain down the road anyway.

[rwmj]

Also don't allow commits which break "git bisect". There are two ways to deal with this, either force all commits to be complete, or document that some patches will need to be squashed together before they are committed to the upstream repository.

[dlrowolleh]

This could be avoided by not merging into your master branch, and having a feature branch, from which subsequent branches are made. Then the dev can open pull requests to merge to the tertiary branch into the secondary one without affecting master.

[yazaddaruvala]

If your team uses feature flags, and it should.

The only healthy thing to do, is to set a deadline for each. When the deadline comes around, the code should be deleted, or a very valid reason should be brought up to extend the deadline.

[wyuenho]

Your last 3 points, especially the last point seems to require a lot of discipline to pull off.

There's an article IBM's developerWorks that says code reviews are the most effective when devs are reviewing 2-400 lines of code at a time. However, when you have team members spanned around the globe, it's difficult to get quick feedback, and obviously one shouldn't wait until there's feedback. Some people on my team will also open up a PR really early. What I'm saying is, there are a lot of roadblocks to review the code your team members many time zones apart and still give them the feedback they need on a timely manner. How would you adjust the approach described above to fit team members that are 7-8 timezones aparts?

http://www.ibm.com/developerworks/rational/library/11-proven...

[cjcenizal]

I'm based in LA and work with a team in Germany. I think we're most productive when we devote the morning to reviewing code and the afternoon to writing code that can be put into a PR by EOD. This way nobody is blocked. It does require a ton of discipline though because if you don't do the morning review you will fall behind and basically get stuck perpetually reviewing PRs without any time for writing code.

[vollmond]

Is that review lopsidedness on purpose? IE, do devs go in and choose to do an arbitrary set of reviews when they feel like it? or are they assigned?

The one code-review-heavy team I worked on did a modified round-robin style. The team wiki had a list of all devs and a count for how many reviews they'd done during the current (3-month) phase. You go in, choose a review with the lowest count, increment their count, then assign them your review. This way, they aren't available again until anyone else who had their count has also done a review.

You could still assign big reviews to senior devs and small reviews to juniors, because there would usually be multiple people with the "lowest" count -- ideally, at some point every dev would have 10 and would then gradually all increment to 11.

[tamana]

Wowz why hasn't anyone automated the process of counting reviews?

[beefsack]

We've achieve a similar thing, but through the use of feature branches. Tickets are branched off a feature branch and merge requests are made back into it which are reviewed one by one.

There is still a final review when merging the feature branch into the upstream branches, but it's merely a scan and not an in-depth review.

[pyro2927]

Have you had any issues with some parallel features overlap in their changes? IE: messy merge conflicts in models after two teams took base functionality in different directions?

[that_guy__]

  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.

[KuhlMensch]

^ Skimming through the page I thought this was an example of a bad PR comment ;)