9 ms·

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

182 points by beliu 8y ago

colemorrison 8y ago
In the "How it all started section" - > E explained that the main impetus behind the introduction of code review was to force developers to write code that other developers could understand; this was deemed important since code must act as a teacher for future developers. This should always be at the forefront of the code reviewer's mindset. It's quite easy for the review process to degenerate into a style argument, quest to find every inefficiency, or attempt to make code as "clever" and brief as possible.
- romed 8y ago
  I think a key to instilling positive code review culture is to make sure that the comments are based on some objective, such as readability and clarity, and not based on personal preference. In particular, if one person is reviewing code of another, it is not helpful for the reviewer to remark that they would have written it differently, if they had written it at all. Some people for some reason can't keep themselves from doing this. They should remember that they did not, in fact, write the change, so their personal style is irrelevant. If code is clear, obviously correct, well-tested, and correctly formatted, it LGTM.
  - gear54rus 8y ago
    except the definition of readability varies and we're back to square one
    - romed 8y ago
      Yes well in the final analysis all problems are recruiting problems, aren’t they?
    - BurningFrog 8y ago
      Not really. Between serious professionals, there will be a few disagreements, but (1) you can work through them with some give and take, and (2) over time you get to know each other, which usually means one persons agrees the other's style is better, or you agree to to disagree on on point.
    - dirkgently 8y ago
      Unless companywide guidelines are agreed upon, and published. E.g http://google.github.io/styleguide/javaguide.html http://google.github.io/styleguide/javaguide.html https://github.com/google/styleguide/blob/gh-pages/pyguide.md https://github.com/google/styleguide/blob/gh-pages/pyguide.m...
      - mikewhy 8y ago
        https://developers.google.com/edu/python/introduction https://developers.google.com/edu/python/introduction: According to the official Python style guide (PEP 8), you should indent with 4 spaces. (Fun fact: Google's internal style guideline dictates indenting by 2 spaces!) https://github.com/google/styleguide/blob/gh-pages/pyguide.md#34-indentation https://github.com/google/styleguide/blob/gh-pages/pyguide.m...: Indent your code blocks with 4 spaces.
  - deleted 8y ago
    [deleted]
  - falsedan 8y ago
    I think putting opinions in code review comments is fine, as long as you’re clearly indicating it as an opinion. The worst is when a reviewer tries to justify their opinionated comment with some vague appeal to style or readability instead of “I hate this part, ship it”
- specialist 8y ago
  Reading another human's code (or prose) is the closest thing we have to mind reading. Whenever I can't avoid doing code reviews, most often, I really wish I didn't know that much about the author. -- "...code must act as a teacher..." Agree. Alas, most teachers aren't very good. https://en.wikipedia.org/wiki/Sturgeon's_law https://en.wikipedia.org/wiki/Sturgeon's_law I used to really care about this stuff. I started a design pattern study group 25 (?) years ago. It's still going strong. (Geeks love to eat pizza and argue about the rules. :) ) Professionally, I've all but given up. Any more, I just try to mimic the code style of whatever goo I'm working on. And do whatever kabuki required of me to get my PRs merged. Programming is now my hobby. Whenever I feel the need to code with intent and feeling, I work on a personal project. (For some reason, this reminds me of Al Pacino quote about exercising. Something like "I just lie down until the feeling goes away." Happily, I'm not that bleak.) -- When I am feeling optimistic, I imagine a future where we adopt a hybrid of Pieter Hintjens' Social Architecture and Michael Bryzek's Test in Production. https://legacy.gitbook.com/book/hintjens/social-architecture/details https://legacy.gitbook.com/book/hintjens/social-architecture... https://qconsf.com/sf2017/sf2017/presentation/testing-production-quality-software-faster.html https://qconsf.com/sf2017/sf2017/presentation/testing-produc... TL;DR: Accept all PRs, radically reduce the cost of changes.
  - Aeolun 8y ago
    I think I mostly accept anything that comes my way, except for ones that have obvious, non-blocking fixes. People tell me to be more critical, but there’s no point in holding everything up because someone wrote a triple nested ternary operator. It’s icky, but they, or someone else, will fix it the next time they come across that piece of code. That’s why we have tests.
vlovich123 8y ago
A link that's actually loading for me: https://ai.google/research/pubs/pub47025 https://ai.google/research/pubs/pub47025
mezzode 8y ago
Off topic, this reminded me how much I hate how pdfs are the standard for papers.
- mikelward 8y ago
  And two columns? Really?
- HelloWorldInWS 8y ago
  Off topic, this reminded me how verbose research papers are.
- jokh 8y ago
  What's the alternative?
  - kybernetikos 8y ago
    Html was literally invented for sharing academic research. The fact that it can also be used for shopping, banking and showing your friends your holiday slides is an happy accident.
  - mezzode 8y ago
    HTML makes perfect sense, as another poster said. I've also found Authorea[1] to be pretty promising. [1] https://www.authorea.com/ https://www.authorea.com/
vivaforever 8y ago
I always think code review can't be done perfectly, because the standard of good code can never reach to a common consent, every programmer has their own styles, puts all together, it mostly become garbage.
erikpukinskis 8y ago
I will be glad when people stop using the word “modern” for this kind of thing. It’s an artifact of some imagined distinction we seem to cling to in the computer world, between the present and the past. No one talks about how to write modern novels. They’re just novels. It’s presumed that they are changing over time. And that things are situated in a historical progression. Also, modernism is an actual thing, and it would be nice if we could use the word for that.
- jeffnappi 8y ago
  No doubt. I believe the word they are looking for is contemporary.
  - kirkules 8y ago
    I'd rather say "current", because to me "contemporary" is something like a transitive adjective. Things really should be contemporary with (or contemporaries of) something else.
    - erikpukinskis 8y ago
      I’d rather use an actually descriptive adjective that situates practices in logical space, rather that use a time-relative term that will become meaningless within a year.
pmcollins 8y ago
Most code review ends up being pair/team programming gone horribly awry. There's a lot of ego involved, it's slow and cumbersome, it's emotionally painful, and the metaphor is wrong (a panel of judges inspecting a lone defendant rather than a team collaborating on an engineering artifact). I recently had the opportunity to work with other developers on a project by displaying a laptop onto a giant screen while we all sat on a couch and drank coffee: it was fun and we produced a high quality result. After years of code review, on the other hand, I can say that code review is painful, expensive, and mediocre. I'm hoping that as software 'eats the world' and provides ever more value to larger numbers of people, companies will realize that putting a single developer on an engineering problem, and having the solution be submitted for review to the rest of the team after the solution has been designed is both unkind to your employees and bad business.
- solipsism 8y ago
  If you have no steps in between assignment and code review, then the problem is not code review. companies will realize that putting a single developer on an engineering problem, and having the solution be submitted for review to the rest of the team after the solution has been designed is both unkind to your employees and bad business. Let's give people a little agency here. "Companies" can't realize anything. If you, as a software engineer, are operating this way and blaming it on "the company" then you are part of the problem. You are the company. Do it better and it'll be better. External locus of control, and using "the company" as a scapegoat to absolve oneself of responsibility, is what's ailing such a company.
  - skybrian 8y ago
    Just like code review doesn't happen on its own, pair programming does need to be a team effort. A single developer deciding to do pair-programming more tends not to change an entrenched culture. Working in the same place and time and on the same code change doesn't happen automatically, particularly in places that support flextime and working from home. The one place I worked where we pair programmed all the time and it worked well was a startup where everyone hired knew going in that it's an XP shop (and this was part of the attraction). On the other hand, flextime and working from home actually are important benefits. It's a tradeoff.
deleted 8y ago
[deleted]
deleted 8y ago
[deleted]
ridiculous_fish 8y ago
Xoogler here, circa 2014. Noogler orientation impressed upon us that every change was code reviewed, period! However my experience was that code reviews were heavyweight and frustrating, often to the point of just being skipped: completely different from the article's observation of latency in hours and "fast-paced iterative development." 1. My initial team was a firmware engineer, and myself doing Android development. My peer felt unqualified to review Android code, so I spent a lot of energy begging code review time from tangentially related teams. Their feedback was not deep or useful for obvious reasons, and the days of latency posed a major problem because our project was tied to a hardware schedule. I learned later my teammate was simply stamping his own changes, which the tool permits, and so I started doing the same. Bypassing code review resolved the problem. 2. My next team was mostly in East Asia, while I was in Mountain View. If they had a question on a change, it would add two days latency due to time zone differences. And because I was physically detached from my peers, I believe they felt more comfortable deferring my changes. Again we were tied to a hardware schedule, so this latency was very painful. 3. The last scenario was overseeing builds, on (say) a Foxconn factory floor. For every build, the hardware coming off the line would behave unexpectedly, and we would have to very quickly update some code to make things work. Code reviews in this situation would have been absurd: any changes only had to last as long as the build itself, and well, there wasn't even network access. This is admittedly an exotic and specialized scenario (but recall orientation's "every change, period!") As a Googler code reviews were either skipped or were a major burden. I made some mistakes: I ought to have escalated quicker, increased my visibility, been less cowed by the orientation. Google also could improve in some areas: 1. Code review expectations need to be adjusted for small heterogenous teams. Such teams are more likely as Google ramps up its hardware efforts. 2. The engineering culture ought to recognize the burden imposed by time zones. Code reviews must be more forgiving when the communication cost is high. 3. Shift orientation towards teams instead of company-wide. CR for self-driving cars must be fundamentally different than CR for matching selfies with famous artwork, or CRs for updating factory floor test fixtures. Some of these may have changed since I left and if so I'd love to hear how.
- cpeterso 8y ago
  Mozilla requires code review for all code changes, too. Time zones are a big challenge because many employees work remotely and few feature teams are collocated in the same office. Engineers have to accept some latency, like you describe. To keep their pipeline full, an engineer typically must multitask multiple bugs in different stages of the review process. Reviewers try to prioritize requests from volunteer contributors so they don't get discouraged. These reviews often require extra feedback on basics of patch creation or coding style. Over time, some experienced volunteer contributors become reviewers, too. Instead of requesting review from a specific individual, some teams have experimented with assigning some review requests to a team alias. Anyone on the team who is available or has some free time can approve and land the patch. This works well for small patches that don't depend on specific expertise. This process also depends on everyone sharing the review burden.
joatmon-snoo 8y ago
Putting the human code review aside, one of the things I've been more impressed by during my time at Google has been the automated presubmits, generally the static analysis ones. I do a lot of Java, so Error Prone (https://errorprone.info/bugpatterns https://errorprone.info/bugpatterns) has been a useful one (cf https://errorprone.info/bugpattern/FormatString); https://errorprone.info/bugpattern/FormatString); we have some others that Tricorder runs but I can't seem to find public references to. One thing that's also an interesting challenge is the work it takes to make something a blocking presubmit: sometimes you don't get useful feedback from one, or maybe a test doesn't provide very good signal, and then people start force submitting to bypass the check because it's not providing utility, but in the process also skip other checks.
- andygrunwald 8y ago
  What so you mean by automated presubmits? Are those „just“ CI checks for static analysis or is there more sophisticated things going on like merging it to a generated branch, deploying it, running all tests in the infrastructure there and more? Can you elaborate a little bit more on this? Or point me to a resource that is doing this?
beefheart 8y ago
Unpopular (?) Opinion: In most "modern" environments, with constant online updates, where shipped defects are not that costly anymore, code review is a net loss. You can just skip it. This study fails to compare with "no code review whatsoever" and it measures by "defects found" instead of "total effort spent" (i.e. the only metric that truly matters). In code review, most developers will not do the kind of thorough analysis that uncovers serious bugs, if they actually did, code review would be too costly. Instead, disagreements on superficialities and bike-shed arguments are promoted, often leading to low-value follow-up changes that can easily cause worse issues than they solve. Code review satisfies the urge for process and structure, but it also satisfies the need for diffusion of responsibility, which is a bad thing. People need to own their code. Code reviews have some value during onboarding or with junior devs, to get everybody on the same page. After that, you can disregard it, the effort is better spent on other things like automated testing.
- badlucklottery 8y ago
  >In code review, most developers will not do the kind of thorough analysis that uncovers serious bugs, if they actually did, code review would be too costly. The point isn't to go in-depth pick apart the design (if a dev needs that kind of deep intervention on a regular basis, it's time to have some hard conversations), it's to share knowledge. The reviewer gets to see a another dev's code along with a good description of what that change does so if there's an emergency someone on the team might actually know where to look. And the submitter gets to know of any footguns the reviewer has seen in similar code. >Instead, disagreements on superficialities and bike-shed arguments are promoted, often leading to low-value follow-up changes that can easily cause worse issues than they solve. Which is why it's important to have a system where the submitter can choose the reviewer(s) to avoid bike-shedding and also choose to ignore the reviewers' comments. That's Google's system. Is someone throwing a lot of BS nits at you? Ack the comments, take them off the reviewers list and pick someone else. Do that a couple times and most folks get the message. Talk to management about those that don't.
  - beefheart 8y ago
    > The reviewer gets to see a another dev's code along with a good description of what that change does so if there's an emergency someone on the team might actually know where to look. In an emergency, you need developers to be able to figure things out regardless of whether they have seen something in a code review or not. Knowledge of a code base is built much more efficiently by actually working on it and, to a lesser degree, reading the code. With the time you save not doing reviews, you can let people write tests or do refactorings to build that exact skill directly, instead of having it be a side-effect.
foreigner 8y ago
12 interviews and 44 survey responses? Is this a joke?
lmilcin 8y ago
After years of trying to make it work at various companies I have stopped supporting "modern code review" implementations. There is a host of problems with those in my opinions, and there are better alternatives. MCR are typically practiced as very shallow analysis by a colleague. It is even solidified in the tyical feedback given as "Looks Good To Me". This is not even remotely near to what is needed to get quality results. Most problems discovered this way are very easy problems that jump right at you that could have been prevented with just a bit more focus on the initial implementation. The reviewer is typically prevented from doing the code review correctly. She is typically engrossed in some other problem when the review comes requiring her to switch context. Aside from the annoyance factor, the typical developer will want to get to his original problem as quickly as possible. Even more annoyingly, people will treat review notes personally and it is very difficult to be the person that does most of the reviews for your team and hence constantly point out errors in their judgement. The reviewer is forced into needlessly artificial role of having to reject/accept colleagues' results with little time for analysis and facing the consequences of being on the other end of the stick the next time. There is no incentive to spend required time on code review. Organizations typically are not willing to support this expense of time even though everybody is willing to support the concept of the review. The review is typically done AFTER the implementation is already completed. This is possibly the worst moment to give the feedback, when the original developer is pressed for time to get his product shipped and where typically very little can be fixed. This causes people to get defensive which is straight way into conflict. It takes effort to avoid conflict if you try to be dilligent in your reviews which is the whole point. So if you don't like how the solution is structured it is typically too late to fix it and there is incentive to just let it go. Finally, the code review typically delays the deployment of the solution as it sits in queue. This is against the agile idea of getting stuff shipped as fast as possible, hopefully immediately. Much better strategy, one that I am promoting now, is pair programming where you physically sit with the other developer and spend time discussing the problem, the requirements, the solution and then implementation and verification. No work is allowed to be done outside of the pair. The problem and requirements are analyzed by both persons so they both understand it well. The implementation is done by both people so they have equal, real chance of catching problems. The possible problems are caught early in the process even at the analysis stage, preventing from having large amounts of work done before review. The discussion is friendly and does not force one of the developers into artificial position where she has to accept or reject the solution. The developers take full responsibility for the implementation as they understand when they ship the code there will be no further review to catch errors -- only (hopefully) automated testing. Finally, when the implementation is done it is done and immediately available for shipment which means the process is not delayed.
dochtman 8y ago
I'm surprised by the finding that Google changes have a medium sized of 24 lines changed. While I'm a big fan of small commits, I was under the impression that some of the large open source Google projects (Chrome, Android) have relatively large commits flowing into their repositories.
- y0ghur7_xxx 8y ago
  I am sure they are rebased several times before going public
jacksmith21006 8y ago
Love Google shares this type of stuff. But why do they? I have been visiting Zircon each morning and seeing what is updated as they develop the new kernel in the open. Just love they do this but why do they?

182 points by beliu 8y ago

In the "How it all started section" - > E explained that the main impetus behind the introduction of code review was to force developers to write code that other developers could understand; this was deemed important since code must act as a teacher for future developers. This should always be at the forefront of the code reviewer's mindset. It's quite easy for the review process to degenerate into a style argument, quest to find every inefficiency, or attempt to make code as "clever" and brief as possible.

romed 8y ago
I think a key to instilling positive code review culture is to make sure that the comments are based on some objective, such as readability and clarity, and not based on personal preference. In particular, if one person is reviewing code of another, it is not helpful for the reviewer to remark that they would have written it differently, if they had written it at all. Some people for some reason can't keep themselves from doing this. They should remember that they did not, in fact, write the change, so their personal style is irrelevant. If code is clear, obviously correct, well-tested, and correctly formatted, it LGTM.
- gear54rus 8y ago
  except the definition of readability varies and we're back to square one
  - romed 8y ago
    Yes well in the final analysis all problems are recruiting problems, aren’t they?
  - BurningFrog 8y ago
    Not really. Between serious professionals, there will be a few disagreements, but (1) you can work through them with some give and take, and (2) over time you get to know each other, which usually means one persons agrees the other's style is better, or you agree to to disagree on on point.
  - dirkgently 8y ago
    Unless companywide guidelines are agreed upon, and published. E.g http://google.github.io/styleguide/javaguide.html http://google.github.io/styleguide/javaguide.html https://github.com/google/styleguide/blob/gh-pages/pyguide.md https://github.com/google/styleguide/blob/gh-pages/pyguide.m...
    - mikewhy 8y ago
      https://developers.google.com/edu/python/introduction https://developers.google.com/edu/python/introduction: According to the official Python style guide (PEP 8), you should indent with 4 spaces. (Fun fact: Google's internal style guideline dictates indenting by 2 spaces!) https://github.com/google/styleguide/blob/gh-pages/pyguide.md#34-indentation https://github.com/google/styleguide/blob/gh-pages/pyguide.m...: Indent your code blocks with 4 spaces.
- deleted 8y ago
  [deleted]
- falsedan 8y ago
  I think putting opinions in code review comments is fine, as long as you’re clearly indicating it as an opinion. The worst is when a reviewer tries to justify their opinionated comment with some vague appeal to style or readability instead of “I hate this part, ship it”
specialist 8y ago
Reading another human's code (or prose) is the closest thing we have to mind reading. Whenever I can't avoid doing code reviews, most often, I really wish I didn't know that much about the author. -- "...code must act as a teacher..." Agree. Alas, most teachers aren't very good. https://en.wikipedia.org/wiki/Sturgeon's_law https://en.wikipedia.org/wiki/Sturgeon's_law I used to really care about this stuff. I started a design pattern study group 25 (?) years ago. It's still going strong. (Geeks love to eat pizza and argue about the rules. :) ) Professionally, I've all but given up. Any more, I just try to mimic the code style of whatever goo I'm working on. And do whatever kabuki required of me to get my PRs merged. Programming is now my hobby. Whenever I feel the need to code with intent and feeling, I work on a personal project. (For some reason, this reminds me of Al Pacino quote about exercising. Something like "I just lie down until the feeling goes away." Happily, I'm not that bleak.) -- When I am feeling optimistic, I imagine a future where we adopt a hybrid of Pieter Hintjens' Social Architecture and Michael Bryzek's Test in Production. https://legacy.gitbook.com/book/hintjens/social-architecture/details https://legacy.gitbook.com/book/hintjens/social-architecture... https://qconsf.com/sf2017/sf2017/presentation/testing-production-quality-software-faster.html https://qconsf.com/sf2017/sf2017/presentation/testing-produc... TL;DR: Accept all PRs, radically reduce the cost of changes.
- Aeolun 8y ago
  I think I mostly accept anything that comes my way, except for ones that have obvious, non-blocking fixes. People tell me to be more critical, but there’s no point in holding everything up because someone wrote a triple nested ternary operator. It’s icky, but they, or someone else, will fix it the next time they come across that piece of code. That’s why we have tests.

vlovich123 8y ago

A link that's actually loading for me: https://ai.google/research/pubs/pub47025 https://ai.google/research/pubs/pub47025

mezzode 8y ago

Off topic, this reminded me how much I hate how pdfs are the standard for papers.

mikelward 8y ago
And two columns? Really?
HelloWorldInWS 8y ago
Off topic, this reminded me how verbose research papers are.
jokh 8y ago
What's the alternative?
- kybernetikos 8y ago
  Html was literally invented for sharing academic research. The fact that it can also be used for shopping, banking and showing your friends your holiday slides is an happy accident.
- mezzode 8y ago
  HTML makes perfect sense, as another poster said. I've also found Authorea[1] to be pretty promising. [1] https://www.authorea.com/ https://www.authorea.com/

vivaforever 8y ago

I always think code review can't be done perfectly, because the standard of good code can never reach to a common consent, every programmer has their own styles, puts all together, it mostly become garbage.

erikpukinskis 8y ago

I will be glad when people stop using the word “modern” for this kind of thing. It’s an artifact of some imagined distinction we seem to cling to in the computer world, between the present and the past. No one talks about how to write modern novels. They’re just novels. It’s presumed that they are changing over time. And that things are situated in a historical progression. Also, modernism is an actual thing, and it would be nice if we could use the word for that.

jeffnappi 8y ago
No doubt. I believe the word they are looking for is contemporary.
- kirkules 8y ago
  I'd rather say "current", because to me "contemporary" is something like a transitive adjective. Things really should be contemporary with (or contemporaries of) something else.
  - erikpukinskis 8y ago
    I’d rather use an actually descriptive adjective that situates practices in logical space, rather that use a time-relative term that will become meaningless within a year.

pmcollins 8y ago

Most code review ends up being pair/team programming gone horribly awry. There's a lot of ego involved, it's slow and cumbersome, it's emotionally painful, and the metaphor is wrong (a panel of judges inspecting a lone defendant rather than a team collaborating on an engineering artifact). I recently had the opportunity to work with other developers on a project by displaying a laptop onto a giant screen while we all sat on a couch and drank coffee: it was fun and we produced a high quality result. After years of code review, on the other hand, I can say that code review is painful, expensive, and mediocre. I'm hoping that as software 'eats the world' and provides ever more value to larger numbers of people, companies will realize that putting a single developer on an engineering problem, and having the solution be submitted for review to the rest of the team after the solution has been designed is both unkind to your employees and bad business.

solipsism 8y ago
If you have no steps in between assignment and code review, then the problem is not code review. companies will realize that putting a single developer on an engineering problem, and having the solution be submitted for review to the rest of the team after the solution has been designed is both unkind to your employees and bad business. Let's give people a little agency here. "Companies" can't realize anything. If you, as a software engineer, are operating this way and blaming it on "the company" then you are part of the problem. You are the company. Do it better and it'll be better. External locus of control, and using "the company" as a scapegoat to absolve oneself of responsibility, is what's ailing such a company.
- skybrian 8y ago
  Just like code review doesn't happen on its own, pair programming does need to be a team effort. A single developer deciding to do pair-programming more tends not to change an entrenched culture. Working in the same place and time and on the same code change doesn't happen automatically, particularly in places that support flextime and working from home. The one place I worked where we pair programmed all the time and it worked well was a startup where everyone hired knew going in that it's an XP shop (and this was part of the attraction). On the other hand, flextime and working from home actually are important benefits. It's a tradeoff.

deleted 8y ago

[deleted]

deleted 8y ago

[deleted]

ridiculous_fish 8y ago

Xoogler here, circa 2014. Noogler orientation impressed upon us that every change was code reviewed, period! However my experience was that code reviews were heavyweight and frustrating, often to the point of just being skipped: completely different from the article's observation of latency in hours and "fast-paced iterative development." 1. My initial team was a firmware engineer, and myself doing Android development. My peer felt unqualified to review Android code, so I spent a lot of energy begging code review time from tangentially related teams. Their feedback was not deep or useful for obvious reasons, and the days of latency posed a major problem because our project was tied to a hardware schedule. I learned later my teammate was simply stamping his own changes, which the tool permits, and so I started doing the same. Bypassing code review resolved the problem. 2. My next team was mostly in East Asia, while I was in Mountain View. If they had a question on a change, it would add two days latency due to time zone differences. And because I was physically detached from my peers, I believe they felt more comfortable deferring my changes. Again we were tied to a hardware schedule, so this latency was very painful. 3. The last scenario was overseeing builds, on (say) a Foxconn factory floor. For every build, the hardware coming off the line would behave unexpectedly, and we would have to very quickly update some code to make things work. Code reviews in this situation would have been absurd: any changes only had to last as long as the build itself, and well, there wasn't even network access. This is admittedly an exotic and specialized scenario (but recall orientation's "every change, period!") As a Googler code reviews were either skipped or were a major burden. I made some mistakes: I ought to have escalated quicker, increased my visibility, been less cowed by the orientation. Google also could improve in some areas: 1. Code review expectations need to be adjusted for small heterogenous teams. Such teams are more likely as Google ramps up its hardware efforts. 2. The engineering culture ought to recognize the burden imposed by time zones. Code reviews must be more forgiving when the communication cost is high. 3. Shift orientation towards teams instead of company-wide. CR for self-driving cars must be fundamentally different than CR for matching selfies with famous artwork, or CRs for updating factory floor test fixtures. Some of these may have changed since I left and if so I'd love to hear how.

cpeterso 8y ago
Mozilla requires code review for all code changes, too. Time zones are a big challenge because many employees work remotely and few feature teams are collocated in the same office. Engineers have to accept some latency, like you describe. To keep their pipeline full, an engineer typically must multitask multiple bugs in different stages of the review process. Reviewers try to prioritize requests from volunteer contributors so they don't get discouraged. These reviews often require extra feedback on basics of patch creation or coding style. Over time, some experienced volunteer contributors become reviewers, too. Instead of requesting review from a specific individual, some teams have experimented with assigning some review requests to a team alias. Anyone on the team who is available or has some free time can approve and land the patch. This works well for small patches that don't depend on specific expertise. This process also depends on everyone sharing the review burden.

joatmon-snoo 8y ago

Putting the human code review aside, one of the things I've been more impressed by during my time at Google has been the automated presubmits, generally the static analysis ones. I do a lot of Java, so Error Prone (https://errorprone.info/bugpatterns https://errorprone.info/bugpatterns) has been a useful one (cf https://errorprone.info/bugpattern/FormatString); https://errorprone.info/bugpattern/FormatString); we have some others that Tricorder runs but I can't seem to find public references to. One thing that's also an interesting challenge is the work it takes to make something a blocking presubmit: sometimes you don't get useful feedback from one, or maybe a test doesn't provide very good signal, and then people start force submitting to bypass the check because it's not providing utility, but in the process also skip other checks.

andygrunwald 8y ago
What so you mean by automated presubmits? Are those „just“ CI checks for static analysis or is there more sophisticated things going on like merging it to a generated branch, deploying it, running all tests in the infrastructure there and more? Can you elaborate a little bit more on this? Or point me to a resource that is doing this?

beefheart 8y ago

Unpopular (?) Opinion: In most "modern" environments, with constant online updates, where shipped defects are not that costly anymore, code review is a net loss. You can just skip it. This study fails to compare with "no code review whatsoever" and it measures by "defects found" instead of "total effort spent" (i.e. the only metric that truly matters). In code review, most developers will not do the kind of thorough analysis that uncovers serious bugs, if they actually did, code review would be too costly. Instead, disagreements on superficialities and bike-shed arguments are promoted, often leading to low-value follow-up changes that can easily cause worse issues than they solve. Code review satisfies the urge for process and structure, but it also satisfies the need for diffusion of responsibility, which is a bad thing. People need to own their code. Code reviews have some value during onboarding or with junior devs, to get everybody on the same page. After that, you can disregard it, the effort is better spent on other things like automated testing.

badlucklottery 8y ago
>In code review, most developers will not do the kind of thorough analysis that uncovers serious bugs, if they actually did, code review would be too costly. The point isn't to go in-depth pick apart the design (if a dev needs that kind of deep intervention on a regular basis, it's time to have some hard conversations), it's to share knowledge. The reviewer gets to see a another dev's code along with a good description of what that change does so if there's an emergency someone on the team might actually know where to look. And the submitter gets to know of any footguns the reviewer has seen in similar code. >Instead, disagreements on superficialities and bike-shed arguments are promoted, often leading to low-value follow-up changes that can easily cause worse issues than they solve. Which is why it's important to have a system where the submitter can choose the reviewer(s) to avoid bike-shedding and also choose to ignore the reviewers' comments. That's Google's system. Is someone throwing a lot of BS nits at you? Ack the comments, take them off the reviewers list and pick someone else. Do that a couple times and most folks get the message. Talk to management about those that don't.
- beefheart 8y ago
  > The reviewer gets to see a another dev's code along with a good description of what that change does so if there's an emergency someone on the team might actually know where to look. In an emergency, you need developers to be able to figure things out regardless of whether they have seen something in a code review or not. Knowledge of a code base is built much more efficiently by actually working on it and, to a lesser degree, reading the code. With the time you save not doing reviews, you can let people write tests or do refactorings to build that exact skill directly, instead of having it be a side-effect.

foreigner 8y ago

12 interviews and 44 survey responses? Is this a joke?

lmilcin 8y ago

After years of trying to make it work at various companies I have stopped supporting "modern code review" implementations. There is a host of problems with those in my opinions, and there are better alternatives. MCR are typically practiced as very shallow analysis by a colleague. It is even solidified in the tyical feedback given as "Looks Good To Me". This is not even remotely near to what is needed to get quality results. Most problems discovered this way are very easy problems that jump right at you that could have been prevented with just a bit more focus on the initial implementation. The reviewer is typically prevented from doing the code review correctly. She is typically engrossed in some other problem when the review comes requiring her to switch context. Aside from the annoyance factor, the typical developer will want to get to his original problem as quickly as possible. Even more annoyingly, people will treat review notes personally and it is very difficult to be the person that does most of the reviews for your team and hence constantly point out errors in their judgement. The reviewer is forced into needlessly artificial role of having to reject/accept colleagues' results with little time for analysis and facing the consequences of being on the other end of the stick the next time. There is no incentive to spend required time on code review. Organizations typically are not willing to support this expense of time even though everybody is willing to support the concept of the review. The review is typically done AFTER the implementation is already completed. This is possibly the worst moment to give the feedback, when the original developer is pressed for time to get his product shipped and where typically very little can be fixed. This causes people to get defensive which is straight way into conflict. It takes effort to avoid conflict if you try to be dilligent in your reviews which is the whole point. So if you don't like how the solution is structured it is typically too late to fix it and there is incentive to just let it go. Finally, the code review typically delays the deployment of the solution as it sits in queue. This is against the agile idea of getting stuff shipped as fast as possible, hopefully immediately. Much better strategy, one that I am promoting now, is pair programming where you physically sit with the other developer and spend time discussing the problem, the requirements, the solution and then implementation and verification. No work is allowed to be done outside of the pair. The problem and requirements are analyzed by both persons so they both understand it well. The implementation is done by both people so they have equal, real chance of catching problems. The possible problems are caught early in the process even at the analysis stage, preventing from having large amounts of work done before review. The discussion is friendly and does not force one of the developers into artificial position where she has to accept or reject the solution. The developers take full responsibility for the implementation as they understand when they ship the code there will be no further review to catch errors -- only (hopefully) automated testing. Finally, when the implementation is done it is done and immediately available for shipment which means the process is not delayed.

dochtman 8y ago

I'm surprised by the finding that Google changes have a medium sized of 24 lines changed. While I'm a big fan of small commits, I was under the impression that some of the large open source Google projects (Chrome, Android) have relatively large commits flowing into their repositories.

y0ghur7_xxx 8y ago
I am sure they are rebased several times before going public

jacksmith21006 8y ago

Love Google shares this type of stuff. But why do they? I have been visiting Zircon each morning and seeing what is updated as they develop the new kernel in the open. Just love they do this but why do they?