Live data from Hacker News

A study of Google's code review tooling (Critique)

engineercodex.substack.com

11–20 of 302 posts

Re: A study of Google's code review tooling (Critique)

#11

I generally like the tool. When your code review tool is really nice one unexpected negative is that it can create a culture of nitpicking. Sometimes it’s not worth arguing over small details like variable naming but the tool makes it really easy for things to head in that direction. Sometimes variable naming isn’t a small detail. But sometimes it is and it’s a waste of everyone’s time to argue about it.

It can be tricky to find a balance point here. At Google scale (in time-of-maintenance, not space... Code can last for years and will be worked on by multiple engineers disconnected from the original project), the hard problem of naming things becomes real. I find that it's useful to keep context. Even though one can never predict with certainty, sometimes you can be real confident that some code is prototype that wi…

At not-Google-scale code also often lasts for years; perhaps even more so since there's typically a lot fewer people maintaining it.

I think the key thing is to ask yourself "is this really objectively better yes/no?" before commenting. Not that you can never comment if the answer to that is "no", but quite a lot of the time when the answer is "no" it doesn't really matter and it's just "I would have done it slightly different, but both are fine".

Re: A study of Google's code review tooling (Critique)

#13
I’m always surprised to see the totality of support for this workflow. My biggest gripes were:

- Owners

- Readability review (or really the 18month queue to get Java/python readability)

Although seemingly innocuous, this made maintaining internal libraries very challenging. There was no way to update every call site of your library in an efficient way. Tools like Rosie were added on top so that you could shard your PRs and shepherd many PRs through a Byzantine approval process.

Compared to Facebook, I found it much harder to reach the same levels of productivity given Owners and readability review. I don’t think libraries like React could get developed at Google given how hard it would be to evolve the API surface.

Re: A study of Google's code review tooling (Critique)

#14
> Developers need to make progress; overly difficult reviews can discourage future improvements.

This, to me, is the most important aspect of code review: get out of the way.

If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have written it, get out of the way. If you're saying anything that isn't directly related to a necessary change to the code, get out of the way.

When I review, if there's no bugs, and nothing that's going to affect performance, and it passes tests and works, I approve it. If somebody wants mentorship I'll add my thoughts in a comment along with my approval.

I don't have this kind of authority, but I would also recommend just not requiring a PR for certain changes. Create a guideline so that changes which a person couldn't improve by reviewing or that have no impact on the working product are just merged. More merging, less useless blockage.

Re: A study of Google's code review tooling (Critique)

#15

I generally like the tool. When your code review tool is really nice one unexpected negative is that it can create a culture of nitpicking. Sometimes it’s not worth arguing over small details like variable naming but the tool makes it really easy for things to head in that direction. Sometimes variable naming isn’t a small detail. But sometimes it is and it’s a waste of everyone’s time to argue about it.

Excessive subjective feedback can be soul-crushing.

Agreed; after > 20 years of coding successfully, got hit by a storm of subjective feedback; it totally ruined any joy in development

Re: A study of Google's code review tooling (Critique)

#16
Wait... this looks like Gerrit with extra plugins

The experience can be nice sure, but Gerrit is not something I'm too enthusiastic about

(and yes, Github is better in some aspects, Gerrit is more configurable though and it seems it is smarting tracking "non official" merges)

Re: A study of Google's code review tooling (Critique)

#17
post #11

Earlier quoted context omitted.

It can be tricky to find a balance point here. At Google scale (in time-of-maintenance, not space... Code can last for years and will be worked on by multiple engineers disconnected from the original project), the hard problem of naming things becomes real. I find that it's useful to keep context. Even though one can never predict with certainty, sometimes you can be real confident that some code is prototype that wi…

At not-Google-scale code also often lasts for years; perhaps even more so since there's typically a lot fewer people maintaining it. I think the key thing is to ask yourself "is this really objectively better yes/no?" before commenting. Not that you can never comment if the answer to that is "no", but quite a lot of the time when the answer is "no" it doesn't really matter and it's just "I would have done it slightly…

Good point. Making renames a suggestion, not gating (unless it contradicts the name in some design doc somewhere that another team is relying on not to move) may be the balance point there.

Re: A study of Google's code review tooling (Critique)

#19

I’m always surprised to see the totality of support for this workflow. My biggest gripes were: - Owners - Readability review (or really the 18month queue to get Java/python readability) Although seemingly innocuous, this made maintaining internal libraries very challenging. There was no way to update every call site of your library in an efficient way. Tools like Rosie were added on top so that you could shard your P…

Google3 is a huge codebase of high quality that moves exceptionally quickly, so it just seems like your priors are getting in the way. Complaining that a large-scale change among tens of thousands of software developers requires a tool (Rosie) is sort of the same thing as all those people who complain that it's too hard to cope with having millions of machines in prod, i.e. the type of people who wash out of the company in a few months and then go on to a career of complaining about it online.
Post reply on HN