Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

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

engineercodex.substack.com

Post reply on HN
Print view View original

41–50 of 302 posts

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

#41

Post by codeapprove »

jeffbee wrote: post #9

Satisfaction 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…

I was on a team at Google that used both Critique and GitHub very heavily, so I was able to constantly see the side-by-side and understand the pain engineers faced when doing external code reviews (as a whole, people actually liked working on GitHub).

After I left I created CodeApprove (https://codeapprove.com) to bring a lot of Google's best code review practices to GitHub. It doesn't give you everything Critique did, but I think it brings the same speed, clarity, and focus in a way that's still compatible with the rest of your GitHub workflow.

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

#42

Post by MarkLowenstein »

TheBlight wrote: post #8
dontreact wrote: post #3

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.

Anything subjective that doesn't fix an identifiable execution problem must be explicitly labeled as a suggestion. You don't get first choice over the code because you are asked to be the reviewer. You are there to (1) catch mistakes and (2) teach the other coder if they appear to not know something useful that you do know. If you have a preference about a simple stylistic matter that is not covered by your style guidelines, either put it in the guidelines, or hold your tongue.

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

#44

Post by BytesAndGears »

throwawaaarrgh wrote: post #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 wr…

I like your thought process here, but let me propose an alternative. I’m going to assume that we’re talking about Senior->Senior level reviews here. Junior engineers should obviously always get more guidance. But for Seniors, the alternative is:

Be extremely picky for PRs from new hires, so that they do things the way that your company does them. Make sure they put files in the right places, and adhere to the “flow” of the rest of the codebase. Also the sorts of style that aren’t picked up by a linter — like architecture choices — should be heavily scrutinized to make sure that they fit with your company’s paradigms.

After a while, they’re fully assimilated and you have a coherent style so that everyone is on the same page. Future code reviews go quickly because everyone makes similar architecture decisions and you’re never surprised. This also makes code easier to read if it all follows the same subset of patterns.

I’m not actually sure which way is better, but I think that both options have benefits.

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

#45

Post by jeffbee »

bheadmaster wrote: post #23
jeffbee wrote: post #10

Earlier quoted context omitted.

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.

"Readable" and "misleading" are subjective, and depend on the person's "preconceptions". I disagree that the reviewer comes "without as many preconceptions" - they just come with different preconceptions, the ones they're used to. Programming language is a language like any other, and each person has their own style of writing it, and they'd prefer the rest of the world to use their own style because it's "more reada…

Can't agree here. Often an author is writing code and they start with a tentative function, parameter, variable, or type name, then as the change progresses the semantics of the thing changed but the name stayed the same. Authors won't always see this but reviewers are more likely to notice it.

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

#46

Post by jasonlotito »

yx827ha wrote: post #12

If you want something similar, check out Gerrit: https://www.gerritcodereview.com/ It's open source and used by Android and Chrome.

Yeah, it seems like Gerrit with lots of Google-specific stuff. Not surprised. Used Gerrit for 12 years, and loved it.

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

#47

Post by Eji1700 »

throwawaaarrgh wrote: post #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 wr…

> 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.

While I'm being pedantic here and get your point, I do think it's important that code is following proper styles.

I get that there's a bunch of nitpicky nonsense that goes into code review, but making sure it follows the same style so in 6 months when someone else has to dig through it you don't have to teach a totally different pattern/method or figure out what arcane bullshit they did is important.

And again, I'm sorta assuming this was implied but wanted to throw it out there since it's one of the most annoying things i've ever had to deal with.

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

#48

Post by sanderjd »

onlyrealcuzzo wrote: post #32
throwawaaarrgh wrote: post #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 wr…

Code review at most companies is an ego thing, for reviewers to feel self-important. It's a way for senior people to pieces of crap to everyone underneath them because that's what they put up with for most of their career - and now they feel like it's their turn to be a piece of crap back and perpetuate the cycle. So many industries run on this exact same cycle. At Google, the company does a good job convincing you t…

I never know whether I've just been fortunate in my career or what, but I've just never experienced anything approaching the kind of unkindness that I frequently see people say is common. I dunno, maybe it's just me, but I'm skeptical. I've always worked with professional colleagues who are trying to figure out how to do their best, and have never once seen people behaving in this childish way.

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

#49

Post by hn_throwaway_99 »

throwawaaarrgh wrote: post #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 wr…

Your comment is why I highly recommend using conventional comment labels, https://conventionalcomments.org/#labels, when doing code reviews.

We've internalized them where I work and I think they're really valuable. When it's abundantly clear whether something is a nitpick, suggestion, or a blocking issue, because it literally has a prefix to that effect, it takes a lot of the potential stress and confrontation out of the code review process, on both sides.

Note you also don't need to memorize the labels, but starting with one forces the reviewer to ask themselves the question "Should I really block the merge for this?"

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

#50

Post by charcircuit »

fishnchips wrote: post #26
jeffbee wrote: post #9

Satisfaction 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…

To be fair to GitHub, there has been slow but constant progress over the last few years. I would love it to get at some point to where Critique was when I left Google (almost 10 years ago).

I don't see any desire for them to actually improve it. It's been over a decade and the code you are actually reviewing is not even on the page of a PR. You have go to a separate web page to see it.
Post reply on HN