8 ms·

Modern Code Review: A Case Study at Google (2018) [pdf]

120 points by ra7 6y ago

cosmotic 6y ago
Modern was a poor choice of words for this title. It's three years old at this point and will only become less applicable over time. "A 2018 Investigation Into Code Review at Google" might have aged better.
- tantalor 6y ago
  What has changed in 3 years?
  - cosmotic 6y ago
    Within Google? I suspect enough to make this paper obsolete. The paper itself says: "Employing lightweight, tool-based code review of code changes (aka modern code review)"; I think that substitution was a disservice. Practically all case studies are on modern processes so it's not really worth specifying.
    - wffurr 6y ago
      After a quick scan of the article, I can tell you that not much has changed in the interim.
      - cosmotic 6y ago
        Will that hold true for the rest of time? If not, a more descriptive word would have been better than 'modern'.
        wffurr 6y ago
        I don't follow. "Modern" in this usage obviously means contemporaneous with the article's publication date.
kbenson 6y ago
By means of 12 interviews, a survey with 44 respondents, and the analysis of review logs for 9 million reviewed changes, we investigate motivations behind code review at Google, current practices, and developers’ satisfaction and challenges. While quite a large number of changes, that seems a rather small sample size for interviews, and especially for survey response.
- mediaman 6y ago
  A general pattern of tech folks trying to understand a subject is that they will analyze billions of data points while doing everything in their power to avoid or minimize talking with people.
  - texasbigdata 6y ago
    Perhaps one of those two activities scales easier than the other.
OneMoreGoogler 6y ago
I worked at Google circa 2015, and found the code review process to be actively terrible. I was writing JS, while my sole teammate was a firmware engineer. He was not willing or able to meaningfully review my code, so I had to try to scrounge random reviewers from other teams. They naturally de-prioritized it, so it took forever. And because they were not involved in my project, they would only have local stylistic feedback. Months in, I finally figured out that other engineers were just self-reviewing: you can add yourself as a reviewer and just click merge. So I started doing that too.
- cmrdporcupine 6y ago
  That honestly sounds very atypical. I've been at Google 9 years and never experienced the scenario you describe. You should probably have reached outside of your team for assistance or flagged a manager. Yes, there have been times when code review wasn't super useful, but on the whole I never experienced the kind of dysfunction you're talking about and merging reviews without external review is a pretty big no-no.
  - ggm 6y ago
    2015 was 6 years ago. Google as possibly changed? Doubled in size (or more) by staff? has more rigorous approaches? has more people capable of meaningfully reviewing code?
- joshuamorton 6y ago
  > Months in, I finally figured out that other engineers were just self-reviewing: you can add yourself as a reviewer and just click merge. So I started doing that too. Where? This certainly isn't possible in google3 (and wasn't possible in 2015 either). That said, there are now some changes made to make, in general, the code review process for people without readability and without teammates who have readability more streamlined.
  - OneMoreGoogler 6y ago
    It was in the Android repo, which used Gerrit. I never dealt with readability, because Android did not require it. In fact that was part of their internal recruiting pitch: fed up with readability? Come work on Android! Heh.
dang 6y ago
Discussed at the time: Modern Code Review: A Case Study at Google [pdf] - https://news.ycombinator.com/item?id=18035548 https://news.ycombinator.com/item?id=18035548 - Sept 2018 (64 comments)
mtlynch 6y ago
>In terms of speed, we find that developers have to wait for initial feedback on their change a median time of under an hour for small changes and about 5 hours for very large changes. The overall (all code sizes) median latency for the entire review process is under 4 hours I worked at Google from 2014-2018. I'm extremely skeptical of these numbers. Based on my experience working on three different teams and sending changelists to many other teams outside my own, I'd expect something more like 3-4 hours for small changes (<50 LOC) and 1-2 days for large changelists (500+ LOC). Google has so many automated changelists that I'm wondering if that skewed their numbers. Like if someone just clicks through 10 auto-generated CLs and approves them all, did that count for a bunch of near-instant review cycles? That said, I was a big fan of Google's code review process. I know people complained about the unnecessary level of process and the hassle of getting readability, but I found it all a great way to spread knowledge across teammates and improve programming skills.
- lupire 6y ago
  Even if the median or 90%ile review is quite quick, the slow ones are where all the pain is. Sure, you can split out 9 little cleanups and typo fixes while working on a larger change, but it's still days to get the main change approved.
- jeffbee 6y ago
  I think pocket reviewers probably account for the generally short time. In my day-to-day I usually did not send changes to people who weren’t already primed to receive them and sitting right next to me, so I could turn around and say “I sent you the thing” and they’d review it straight away because we were working towards the same goal anyway, and in some cases had already pair-designed or pair-coded the thing so the review was already a formality.
  - BurningFrog 6y ago
    Pair programming really is the best code review!
    - 6y ago
Goog_l_Dude_l 6y ago
The review experience can really vary between teams, and depending on whether you have readability or not and whether you're changing code you own or not. This can make the difference between a couple of hours to a week to submit the same change. Readability in general is a process that while valuable, but can be really streamlined. I was working in a fast moving team using Python where everyone had readability, and I just didn't have time to delay most non trivial CLs, so even after submitting hundreds of CLs I did not have readability, having sent only a handful for review. Another negative effect I saw was people opting not to use a less popular language that was the right choice for a project, just because not many people in their office have readability in that language. A much easier process would go something like this: Suppose my teammate has readability in language X, They can tag review comments as being readability related or not. Once someone has N CLs under their belt with readability related comments under some low percentage in the past M CLs, they are granted readability automatically, or have to submit just 1-2 CLs for a speedy readability process.
- dilyevsky 6y ago
  Yeah python and c++ readability there is full on bs. No two readability reviewers agree on things, nitpicking was ridiculous and very superficial ime and most of those issues can be solved with some light linting/static analysis. Then you watch all those l6s+ shipping obviously terribly styled code bc noone dares to say shit... fwiw Go had much better process for this but i got mine when the readibility list was like a hundred folks so not sure about now
  - slumpt_ 6y ago
    Google has heaps of horrible code. The problem with lots of small isolated CLs and little time invested in revisiting broader architectures. Then they’ll finally rewrite the thing some day, after a massive and often unnecessary design doc filled with needless bike-shedding that ultimately proposes an overengineered solution to help the person make L+1.
    - Goog_l_Dude_l 6y ago

120 points by ra7 6y ago

Modern was a poor choice of words for this title. It's three years old at this point and will only become less applicable over time. "A 2018 Investigation Into Code Review at Google" might have aged better.

tantalor 6y ago
What has changed in 3 years?
- cosmotic 6y ago
  Within Google? I suspect enough to make this paper obsolete. The paper itself says: "Employing lightweight, tool-based code review of code changes (aka modern code review)"; I think that substitution was a disservice. Practically all case studies are on modern processes so it's not really worth specifying.
  - wffurr 6y ago
    After a quick scan of the article, I can tell you that not much has changed in the interim.
    - cosmotic 6y ago
      Will that hold true for the rest of time? If not, a more descriptive word would have been better than 'modern'.
      - wffurr 6y ago
        I don't follow. "Modern" in this usage obviously means contemporaneous with the article's publication date.

kbenson 6y ago

By means of 12 interviews, a survey with 44 respondents, and the analysis of review logs for 9 million reviewed changes, we investigate motivations behind code review at Google, current practices, and developers’ satisfaction and challenges. While quite a large number of changes, that seems a rather small sample size for interviews, and especially for survey response.

mediaman 6y ago
A general pattern of tech folks trying to understand a subject is that they will analyze billions of data points while doing everything in their power to avoid or minimize talking with people.
- texasbigdata 6y ago
  Perhaps one of those two activities scales easier than the other.

OneMoreGoogler 6y ago

I worked at Google circa 2015, and found the code review process to be actively terrible. I was writing JS, while my sole teammate was a firmware engineer. He was not willing or able to meaningfully review my code, so I had to try to scrounge random reviewers from other teams. They naturally de-prioritized it, so it took forever. And because they were not involved in my project, they would only have local stylistic feedback. Months in, I finally figured out that other engineers were just self-reviewing: you can add yourself as a reviewer and just click merge. So I started doing that too.

cmrdporcupine 6y ago
That honestly sounds very atypical. I've been at Google 9 years and never experienced the scenario you describe. You should probably have reached outside of your team for assistance or flagged a manager. Yes, there have been times when code review wasn't super useful, but on the whole I never experienced the kind of dysfunction you're talking about and merging reviews without external review is a pretty big no-no.
- ggm 6y ago
  2015 was 6 years ago. Google as possibly changed? Doubled in size (or more) by staff? has more rigorous approaches? has more people capable of meaningfully reviewing code?
joshuamorton 6y ago
> Months in, I finally figured out that other engineers were just self-reviewing: you can add yourself as a reviewer and just click merge. So I started doing that too. Where? This certainly isn't possible in google3 (and wasn't possible in 2015 either). That said, there are now some changes made to make, in general, the code review process for people without readability and without teammates who have readability more streamlined.
- OneMoreGoogler 6y ago
  It was in the Android repo, which used Gerrit. I never dealt with readability, because Android did not require it. In fact that was part of their internal recruiting pitch: fed up with readability? Come work on Android! Heh.

dang 6y ago

Discussed at the time: Modern Code Review: A Case Study at Google [pdf] - https://news.ycombinator.com/item?id=18035548 https://news.ycombinator.com/item?id=18035548 - Sept 2018 (64 comments)

mtlynch 6y ago

>In terms of speed, we find that developers have to wait for initial feedback on their change a median time of under an hour for small changes and about 5 hours for very large changes. The overall (all code sizes) median latency for the entire review process is under 4 hours I worked at Google from 2014-2018. I'm extremely skeptical of these numbers. Based on my experience working on three different teams and sending changelists to many other teams outside my own, I'd expect something more like 3-4 hours for small changes (<50 LOC) and 1-2 days for large changelists (500+ LOC). Google has so many automated changelists that I'm wondering if that skewed their numbers. Like if someone just clicks through 10 auto-generated CLs and approves them all, did that count for a bunch of near-instant review cycles? That said, I was a big fan of Google's code review process. I know people complained about the unnecessary level of process and the hassle of getting readability, but I found it all a great way to spread knowledge across teammates and improve programming skills.

lupire 6y ago
Even if the median or 90%ile review is quite quick, the slow ones are where all the pain is. Sure, you can split out 9 little cleanups and typo fixes while working on a larger change, but it's still days to get the main change approved.
jeffbee 6y ago
I think pocket reviewers probably account for the generally short time. In my day-to-day I usually did not send changes to people who weren’t already primed to receive them and sitting right next to me, so I could turn around and say “I sent you the thing” and they’d review it straight away because we were working towards the same goal anyway, and in some cases had already pair-designed or pair-coded the thing so the review was already a formality.
- BurningFrog 6y ago
  Pair programming really is the best code review!
  - 6y ago

Goog_l_Dude_l 6y ago

The review experience can really vary between teams, and depending on whether you have readability or not and whether you're changing code you own or not. This can make the difference between a couple of hours to a week to submit the same change. Readability in general is a process that while valuable, but can be really streamlined. I was working in a fast moving team using Python where everyone had readability, and I just didn't have time to delay most non trivial CLs, so even after submitting hundreds of CLs I did not have readability, having sent only a handful for review. Another negative effect I saw was people opting not to use a less popular language that was the right choice for a project, just because not many people in their office have readability in that language. A much easier process would go something like this: Suppose my teammate has readability in language X, They can tag review comments as being readability related or not. Once someone has N CLs under their belt with readability related comments under some low percentage in the past M CLs, they are granted readability automatically, or have to submit just 1-2 CLs for a speedy readability process.

dilyevsky 6y ago
Yeah python and c++ readability there is full on bs. No two readability reviewers agree on things, nitpicking was ridiculous and very superficial ime and most of those issues can be solved with some light linting/static analysis. Then you watch all those l6s+ shipping obviously terribly styled code bc noone dares to say shit... fwiw Go had much better process for this but i got mine when the readibility list was like a hundred folks so not sure about now
- slumpt_ 6y ago
  Google has heaps of horrible code. The problem with lots of small isolated CLs and little time invested in revisiting broader architectures. Then they’ll finally rewrite the thing some day, after a massive and often unnecessary design doc filled with needless bike-shedding that ultimately proposes an overengineered solution to help the person make L+1.
  - Goog_l_Dude_l 6y ago