4 ms·

One thing Google has going for it to make code review more efficient is the concept of a uniform per-language style guide. The style guides are absolute and com

One thing Google has going for it to make code review more efficient is the concept of a uniform per-language style guide. The style guides are absolute and comprehensive, meaning it’s not permitted to diverge from them, and equally importantly, that any style topics not covered by the style guide are not considered legitimate style issues to raise in code review. Formatting can be almost entirely automated with pre-submit checks that the code is formatted, and automatic formatting tools. So for your two examples, this practice of adopting a style guide prevents them from arising. The “tabs vs spaces” argument is simply settled, because the style guide has an answer. The “for loop vs for-each loop” is either addressed by the style guide, in which case there is an answer, or it’s not, in which case it is not a legitimate style complaint one way or the other, and the reviewer is making a mistake by bringing it up. https://google.github.io/styleguide/ https://google.github.io/styleguide/

elischleifer 4y ago
I would say any styleguide that is in prose form and not machine enforced is deficient. Modern linting and formatting tools are the best, most efficeiect means for enforcing style. No religious arguments are needed if the tooling decides what is correct. Shameless plug for https://trunk.io/products/check https://trunk.io/products/check - which will handle universal enforcement of all the tooling for all of the pieces of your tech stack.
0x20cowboy 4y ago
Yeah sorry I was just trying to pull something silly out of the air - we do mostly go and gofmt (or clangd or whatever). I was just trying to flowerly highlight that most “in depth code reviews” I’ve seen are very focused on surface level things. I’d dig a tool that focused more on structural changes.
- jeffreygoesto 4y ago
  Well. One could argue that if you have to discuss these very deep topics in a PR, you better had brought them up before coding that stuff wrong in the first place. Oh, that is not considered agile you say? Discussing these deep topics is also socially very challenging and you need some quite grown up people to discuss them (that late after the damage has been done) without awkward "but I spent all this time" moments and at least some personal animosity. But the grown ups think and discuss first typically, which also might be why you don't see them discuss those topics in PRs...
  - 0x20cowboy 4y ago
    > One could argue that if you have to discuss these very deep topics in a PR, you better had brought them up before coding that stuff wrong in the first place. Fair point, and 100% on the nose. The times when this comes in handy is when you have to PR on a project you don’t know a lot about (which could be argued is a process problem, but I am getting way off trail now). However, with your excellent point in mind, I am even more confused on what a “culture of rigorous code reviews” is.
  - sleepydog 4y ago
    It's really nice to have these discussions in person with the person sitting next to you. That's probably what I missed the most during the pandemic--impromptu code/design discussions. Screen sharing works but it's less spontaneous and natural.
- UncleMeat 4y ago
  It is a culture thing. People who receive reviews that address issues of design, structure, maintainability, and abstraction will learn to do the same for others. This is hard to establish but once it is there it sticks. Early stories about how some Googlers managed to shift the entire company to seeing code review as normal and expected are fascinating. Google also has documentation, guidance, and training on how to review code effectively.
ertian 4y ago
On top of that, reviewing is considered an important and valuable skill. You have to demonstrate a deep understanding of the language and go through a months-long meta-review process before you can even start doing (language, as opposed to business logic) reviews, and being an authorized reviewer is a legit achievement when you're going for promo. Basically: reviews are actually treated as important.

elischleifer 4y ago

I would say any styleguide that is in prose form and not machine enforced is deficient. Modern linting and formatting tools are the best, most efficeiect means for enforcing style. No religious arguments are needed if the tooling decides what is correct. Shameless plug for https://trunk.io/products/check https://trunk.io/products/check - which will handle universal enforcement of all the tooling for all of the pieces of your tech stack.

0x20cowboy 4y ago

Yeah sorry I was just trying to pull something silly out of the air - we do mostly go and gofmt (or clangd or whatever). I was just trying to flowerly highlight that most “in depth code reviews” I’ve seen are very focused on surface level things. I’d dig a tool that focused more on structural changes.

jeffreygoesto 4y ago
Well. One could argue that if you have to discuss these very deep topics in a PR, you better had brought them up before coding that stuff wrong in the first place. Oh, that is not considered agile you say? Discussing these deep topics is also socially very challenging and you need some quite grown up people to discuss them (that late after the damage has been done) without awkward "but I spent all this time" moments and at least some personal animosity. But the grown ups think and discuss first typically, which also might be why you don't see them discuss those topics in PRs...
- 0x20cowboy 4y ago
  > One could argue that if you have to discuss these very deep topics in a PR, you better had brought them up before coding that stuff wrong in the first place. Fair point, and 100% on the nose. The times when this comes in handy is when you have to PR on a project you don’t know a lot about (which could be argued is a process problem, but I am getting way off trail now). However, with your excellent point in mind, I am even more confused on what a “culture of rigorous code reviews” is.
- sleepydog 4y ago
  It's really nice to have these discussions in person with the person sitting next to you. That's probably what I missed the most during the pandemic--impromptu code/design discussions. Screen sharing works but it's less spontaneous and natural.
UncleMeat 4y ago
It is a culture thing. People who receive reviews that address issues of design, structure, maintainability, and abstraction will learn to do the same for others. This is hard to establish but once it is there it sticks. Early stories about how some Googlers managed to shift the entire company to seeing code review as normal and expected are fascinating. Google also has documentation, guidance, and training on how to review code effectively.

ertian 4y ago

On top of that, reviewing is considered an important and valuable skill. You have to demonstrate a deep understanding of the language and go through a months-long meta-review process before you can even start doing (language, as opposed to business logic) reviews, and being an authorized reviewer is a legit achievement when you're going for promo. Basically: reviews are actually treated as important.