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

51–60 of 302 posts

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

#51

Post by compiler-guy »

ASinclair wrote: post #29
tallowen wrote: post #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 P…

> I don’t think libraries like React could get developed at Google given how hard it would be to evolve the API surface. That's more of a feature and not a bug to me. Users of a library would appreciate it if the API surface doesn't change dramatically over time.

That's the strange thing about the GP comment: Teams can and do change their API surface dramatically over time. They use the LSC process that the GP doesn't like to do it, and it is 99% invisible to the API users. It works, if not exactly flawlessly every time, very consistently well, and there are dozens of such changes in flight all the time that API users barely even realize exist.

Don't know what the alternative would be.

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

#52

Post by _ea1k »

sanderjd wrote: post #36
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 appreciate suggestions that are not "directly related to a necessary change to the code", and I believe my coworkers appreciate mine. It is very useful to say "did you consider doing it xyz way / using xyz library?" while also mashing the "approve" button. If they say "yep and I decided not to because of xyz" or "yep I like that idea but I'm not gonna change it at this point" then no harm done. But pretty often in…

> [...] while also mashing the "approve" button

+1 - It is easy to forget that it is possible to approve with comments. Not all review feedback should block a merge.

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

#53

Post by jrockway »

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…

To me the biggest time sink in PR reviews is getting the reviewer to open the code review. If I ever take over the world, I'd make a new OS that locks all activity whenever you get a code review and doesn't let you switch applications until you click "approve" or "request changes".

I find it really crazy how long people take to do reviews. I have Github connected to Slack; if someone needs my review I get a message and can have it done within 5 minutes. (I think I write the most comments on code reviews of anyone in my organization, and I also have the lowest review latency. If I can do it, anyone can do it.)

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

#54

Post by wubrr »

acscott wrote: post #15
TheBlight wrote: post #8

Earlier quoted context omitted.

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

The fun thing to do in these situations is to add yourself as reviewer to all PRs by the person giving such feedback and return the favor. They learn pretty fast.

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

#55

Post by kstrauser »

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…

Note that PRs are mandatory if a company goes through SOC-2 or any other compliance framework. The underlying question they answer is “could a malicious or incompetent employee send bad code to production without another engineer viewing it first?”

The specific response auditors expect is “no, because another human reviews all change requests before they’re approved”.

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

#56

Post by nappy-doo »

jasonlotito wrote: post #46
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.

Gerrit is pretty crap compared to critique. It has a workflow that works for Android, but critique is really much better.

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

#57

Post by cedws »

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…

Well said. Ego should stay out of PRs. Your job as a reviewer is to help the code get merged, not hinder it.

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

#58

Post by Ensorceled »

sanderjd wrote: post #48
onlyrealcuzzo wrote: post #32

Earlier quoted context omitted.

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.

For every unkind reviewer I've encountered in my career, I've encountered at least one person who overreacts to review comments.

I also agree that both have been pretty rare for me.

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

#59

Post by iwontberude »

jrockway wrote: post #53
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…

To me the biggest time sink in PR reviews is getting the reviewer to open the code review. If I ever take over the world, I'd make a new OS that locks all activity whenever you get a code review and doesn't let you switch applications until you click "approve" or "request changes". I find it really crazy how long people take to do reviews. I have Github connected to Slack; if someone needs my review I get a message a…

If you link peoples ability to use their computer by how quickly they can click "Approve" on a PR, it will definitely lead to a break down in review quality and beat you at your own personal records of latency and MTTR.

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

#60

Post by arp242 »

shadowgovt wrote: post #17
arp242 wrote: post #11

Earlier quoted context omitted.

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.

But "another team is relying on not to move" is an objective point, right?

Things like "I find this code very hard to follow, and I think it could be made easier" is also objective, and even "I don't understand what this variable name means, and I think it could be clearer".

I once names a function mkdir(). This created a directory tree. In the review it was called "obscure" so it became createDir(). Then someone pointed out that "dir" was a needless abbreviation so it became createDirectory(). Then yet someone else pointed out that it's actually recursive, and a discussion on the merits of createDirectoryRecursively() vs. createRecursiveDirectory() was inflicted on everyone. In-between there was a side-quest about directory vs. folder. Just fucking stick a fork in my eye already.

Was anything of any objective value gained? I'm having a hard time seeing it. Well, it makes for a slightly amusing anecdote so there's that.

Post reply on HN