A study of Google's code review tooling (Critique)
engineercodex.substack.com
A study of Google's code review tooling (Critique)
1–10 of 302 posts
Re: A study of Google's code review tooling (Critique)
#2[flagged]
Re: A study of Google's code review tooling (Critique)
#3I 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.
Re: A study of Google's code review tooling (Critique)
#4That ML-sidecar for recommending the actual change is a very correct place to put that technology: right in the line where someone is already reviewing the code, so they can confirm the ML suggestion looks correct before adding it.
I can't overstate how valuable it is for reviewers to give the actual change they're recommending alongside the description, but it happens too infrequently if we make the reviewer write it every time. Actual code is the cleanest language to communicate ideas, but it's a time-sink and a mental drain to turn the English description into code snippets every time. Automating that is precisely the right place to save time for reviewers.
Re: A study of Google's code review tooling (Critique)
#5Critique is nice but the ML suggested edits are a waste of attention and cant be disabled.
Re: A study of Google's code review tooling (Critique)
#6I 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 will be thrown away in six months. And there's a big difference between the code that makes up an API layer and the code that implements a feature constrained to one module.
Re: A study of Google's code review tooling (Critique)
#7Critique is nice but the ML suggested edits are a waste of attention and cant be disabled.
What's the current miss-rate on them? In principle, it seems brilliant, but I can see that value dropping off a cliff if more than half of them are worthless.
Re: A study of Google's code review tooling (Critique)
#8I 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.
Re: A study of Google's code review tooling (Critique)
#9Satisfaction with Critique among Xooglers is undoubtedly driven by dissatisfaction with GitHub PR reviews. GitHub reviews are astonishingly bad. The tool is utterly useless for actual reviews. After the first round of comments it becomes total chaos. Nobody can tell what's been said, done, changed, or resolved. It is impossible to believe that the people who write and maintain the GitHub PR tools have themselves ever practiced code review.
Re: A study of Google's code review tooling (Critique)
#10I 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.
Writing readable code is not "nitpicking". It is common that an author doesn't see why their parameter name or function name is misleading but their reviewer, who comes to the change without as many preconceptions, sees it right away.