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

91–100 of 302 posts

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

#91

Post by yjftsjthsd-h »

nox100 wrote: post #74
_ea1k wrote: post #52

Earlier quoted context omitted.

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

It is a merge block in some code bases. You need to know the code the person said they were going to commit is the code they actually commit. Especially when there is financial incentive and state actors that want code inserted. In my project we used to be allowed to approve with nits but recently they changed it that the code needs a re-review for almost any edits. The system has some criteria for which it will allo…

I think it's legitimate to force a re-review for any code change, given that programming is one of those charming areas where a single character can completely change the meaning of something.

That said, if you can read the commits individually then final review for something where minor changes were added should be utterly trivial.

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

#92

Post by yjftsjthsd-h »

jrockway wrote: post #70
iwontberude wrote: post #59

Earlier quoted context omitted.

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.

I'm sure there would be negative effects, but at least things would move forward quickly. This is the worst system except for all the others.

Move quickly yes, move forward not necessarily. You've described a system where everybody hits approve or finds something to complain about as fast as they possibly can without worrying about little questions like whether it should have been approved or whether the changes they request are legitimate.

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

#93

Post by yjftsjthsd-h »

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…

> I have Github connected to Slack; if someone needs my review I get a message and can have it done within 5 minutes.

Do you not do anything else? If I'm working on something else, I'm not getting to any new task within 5m (at least, not without a net loss of productivity to reload context)

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

#94

Post by LeafItAlone »

hyggetrold wrote: post #90

I think it's funny that devs will go through all kinds of untold suffering when it comes to code reviews...stacked pull requests, waiting days (or more!) for a review, all kinds of nitpicky BS that is a lot of time and effort to rework by the time you have the code all written. But suggest to people that they pair program and that it's a real-time review that obviates the need for formal async review and folks want t…

I wonder if there are studies on the effectiveness of pair programming vs code review and bugs caught.

Anecdotally, I catch more bugs doing code review than pairing. When pair programming, we naturally start to think similar thoughts, so it doesn’t get a great real second set of fresh eyes. Even when I have pair, I like to do an alone-time code review and things almost always pop up.

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

#95

Post by withinboredom »

dawnerd wrote: post #89
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…

Yeah no, part of code review is ensuring code quality is maintained. If you start letting badly written code fly because it passes tests, that’ll bite you. If something isn’t blocking I’ll make a note that it should be fixed if theres budget knowing sometimes you gotta let things go. But you can’t just ignore code quality and call it nitpicking.

Badly written, or badly designed? Bad code, to me, would be an unrolled for-loop, or a for-loop where a foreach loop would do better (though now, we're getting in the realm of nit-picking). A PR is waaaaay too late to bring up architectural/design improvements (unless it is a WIP PR opened expressly for discussing the approach).

I very rarely see bad code in PR's unless it is from a Junior programmer, and even then, it's usually because they don't know better. Even then, I choose to trust them to do the right thing. In most environments, you can even review the code AFTER it has been merged. So, if they don't address the issue, I can still review the code and let them know that I expect to see a follow-up PR addressing it.

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

#97

Post by dilyevsky »

tallowen wrote: post #73
jeffbee wrote: post #27

Earlier quoted context omitted.

Except Facebook's codebase is an order of magnitude smaller.

What makes you say that Facebook's codebase is an order of magnitude smaller?

Fb had 100 million loc in 2019[0] while google already had 2 billion[1]

[0] -https://www.wired.com/story/facebook-zoncolan-static-analysi...

[1] - https://www.wired.com/2015/09/google-2-billion-lines-codeand...

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

#98

Post by thiht »

withinboredom wrote: post #95
dawnerd wrote: post #89

Earlier quoted context omitted.

Yeah no, part of code review is ensuring code quality is maintained. If you start letting badly written code fly because it passes tests, that’ll bite you. If something isn’t blocking I’ll make a note that it should be fixed if theres budget knowing sometimes you gotta let things go. But you can’t just ignore code quality and call it nitpicking.

Badly written, or badly designed? Bad code, to me, would be an unrolled for-loop, or a for-loop where a foreach loop would do better (though now, we're getting in the realm of nit-picking). A PR is waaaaay too late to bring up architectural/design improvements (unless it is a WIP PR opened expressly for discussing the approach). I very rarely see bad code in PR's unless it is from a Junior programmer, and even then,…

> A PR is waaaaay too late to bring up

No. That’s literally the point of a review.

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

#99

Post by qmarchi »

willsmith72 wrote: post #20

that ui sure is ugly. google really is an engineering company, and i love that, but it's still kind of jarring

It's definitely looking better than even a year ago. There's a lot of internal UI reworks ongoing to bring things to consistency.
Post reply on HN