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

171–180 of 302 posts

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

#171

Post by ravenstine »

withinboredom wrote: post #158
johnnyanmac wrote: post #138

Earlier quoted context omitted.

1. That's the point. It's not up to standard 2. They could but they usually don't. It's fine to sit down and make sure they understand some esoteric structure within a system I owj I'm not going to sit down with someone else to make sure they use for each loops unless it's a very new junior (i wouldn't block that in most code reviews anyway unless the code was legitimately painful to read, but every company is differ…

Anyone who has ever come along in a code review and told me my code “isn’t up to standard” will inevitably start a very long game of attrition with me. Here’s the thing: there’s no such thing as “standard” software architecture (until someone writes a new kind of software architecture called “standard”). I’ve seen and used everything from MVC to DDD to TDD to MVVC to whatever React is to reactive to event-oriented to…

Yeah, gotta love it when someone uses terms like "standard" and "quality" but definitions of those are written nowhere. Quality is almost never objective either; some cultures like "overripe" bananas and some consider the best bananas to have no spots, and those picking bananas need a different basis for what makes a good quality banana to the buyer. Unless programmers diligently document their standards (which they rarely do adequately), they shouldn't be surprised if the other programmers is dumbfounded by being told their code isn't up to standard.

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

#172

Post by ayberk »

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.

I don't know if it's the tool honestly. I've recently switched teams and I'm in the process of getting java readability. The whole experience so far has been much worse than getting my C++ readability. I even get "nit" comments on the code I haven't changed. I have had multiple comments where reviewer basically "preferred" one style over another. It's been still mostly helpful, but I've had my share of frustration.

C++ readability was a much, much better experience. All the comments were about actually making the code better, eg, "use THIS_MACRO() instead of THAT_MACRO(), because go/...".

I guess I think it's much more about the reviewer, and based on my anecdotal experience, the language :)

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

#173

Post by merb »

zeroCalories wrote: post #166
MarkLowenstein wrote: post #42

Earlier quoted context omitted.

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

IMO you should never provide feedback that can be implemented as an automated check. If you don't like deeply nested control flow, then you should catch that with static analysis. If you need code coverage, you should require it for merging. Implement your check and provide a new PR to fix your nitpicks, or shut up. The goal is to put 100% of the focus on correctness.

Sadly typos in variable names are not checkable that easy

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

#174

Post by lazyasciiart »

_ea1k wrote: post #110
nox100 wrote: post #74

Earlier quoted context omitted.

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…

Yes, it is pretty common to require re-review if it is changed again. But approval with comments shifts control back to the submitter. They then have choices: - Modify it and accept that this will trigger re-review - Merge as-is... maybe they don't even agree with the nit - Merge as-is and followup with a new MR under the same JIRA. This sometimes makes sense if it gets them into an important preprod environment, but…

I got a merge blocked last week with the comment 'I think this work is more related to "TFS X: prepare for Y" than it is "TFS Y".' (I eventually realized they wanted me to change the commit message). I hate this fucking team.

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

#175

Post by arp242 »

shadowgovt wrote: post #159
arp242 wrote: post #131

Earlier quoted context omitted.

It's not an exact science and there's a grey area of course, but what I have in mind is mostly code that's just needlessly confusing in ways I think very few would disagree with. I can't really think of a specific non-trivial example at the moment, but just like natural language I do think some code can objectively "hard to follow", even if that's somewhat vague and not strictly defined. Other factors are if I'm the…

I can't speak to everyone's experience, but I can say how teams I've been on have handled that sort of thing. When it's not explicitly documented in the style guide and it doesn't violate some agreed-upon convention (which needs to be gleanable from within the file itself or it's not an agreed-upon convention... No "We know it was always done like that but now we're doing this," if you want to change it, you do the h…

I'm not talking about style, I'm talking about logic. Some logic is easier to follow than others, and as mentioned this is not exact and somewhat fuzzy, but "every logic is equal to every other logic" is obviously nonsense. All other things being equal two "if" statements are better than nine "if" statements.

I don't care about style; as far as I'm concerned it's "do what thou wilt shall be the whole of the style guide" (well, within some obvious limits of reason). I don't even care about consistency all that much.

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

#176

Post by yjftsjthsd-h »

withinboredom wrote: post #102
yjftsjthsd-h wrote: post #93

Earlier quoted context omitted.

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

If you're writing code and you get a review request and ignore it, you are literally preventing value from being shipped while you work on making some abstract idea a reality. Usually, the code review is the very last step before value becomes available to the business. There is absolutely no reason to delay a code review in this case, none, whatsoever. Even if it isn't the last step, you're delaying the chance for a…

> Usually, the code review is the very last step before value becomes available to the business. There is absolutely no reason to delay a code review in this case, none, whatsoever.

Even if I was just working on another feature, that's questionable, because it only works if earlier-stage work might not ship at all; otherwise a delay at the beginning is the same as delay at the end, and overhead from context switching means you should prioritize whatever you're already doing.

But where it really gets absurd to claim that code review is always the highest possible priority is that I never said what I was working on at the time. Maybe I'm working on incident response - "Oh, sorry $BIGGEST_CUSTOMER, I know your prod instance is down and you're losing thousands of dollars per minute, but I just have to pop off for a bit because one of my peers needs me to code review a way to make forms load 0.2 seconds faster!". Maybe I'm working on a different feature, but it's one that will give the company more value (I have been in conversations to the tune of "we need this feature so we can close with this lucrative customer"). Maybe the other thing I'm doing is someone else's code review!

Now to be fair, I think there is real value in lowering the turnaround time on reviews. I could even see an argument for having someone who does code reviews to the exclusion of all else (which kinda sounds like the role you're playing in your company), provided that's known and factored in when handing them work. The only reason I think your position is unreasonable is that you've thrown out any notion of triage or nuance and pinned reviews to the very highest priority above all else.

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

#177

Post by ot »

dilyevsky wrote: post #97
tallowen wrote: post #73

Earlier quoted context omitted.

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

That's an apples-and-oranges comparisons, 100M is just the size of the frontend/middleware code (Hack), while 2B includes everything, including configuration and generated code.

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

#178

Post by andromeduck »

vinkelhake wrote: post #101
compiler-guy wrote: post #28

Worth noting that Google engineers had this level of satisfaction long before the mostly useless AI suggestions came on the scene. Those additions are comparatively recent.

I find them generally very helpful. In a majority of cases, it's able to translate my review comments into the exact delta that I had in mind. Source: me, C++ readability reviewer - this means I review code from lots of different people, all over the codebase.

It takes way too long though - it's almost always quicker to just do it in cider/subl if there's more than 1 of them.

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

#179

Post by mtlynch »

codeapprove wrote: post #33

Shameless but relevant plug: I built CodeApprove ( https://codeapprove.com ) to bring the best parts of the Critique workflow to GitHub PRs. Obviously there are many things that don't translate (Google doesn't use git, and it has insane CI/CD integration) but CodeApprove gives you the same workflow. You always know which conversations are resolved, which PRs need your attention, etc. Feel free to reach out via email…

I'm a former Google developer and current CodeApprove customer, and I just want to endorse CodeApprove. They're the closest thing I've found to Critique outside of Google.

The UI is really straightforward, and Sam (the founder) has been responsive to feedback.[0]

I have no relationship with the company except just being a satisfied customer.

[0] https://github.com/codeapprove/feedback/issues

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

#180

Post by arp242 »

IanCal wrote: post #164
arp242 wrote: post #117

Earlier quoted context omitted.

It was a mix of short and long and there wasn't really much "style". The company had taken over the maintainership from someone else at some point (before I joined) and it was all fairly inconsistent. I don't really mind the inconsistencies as such, it's just the argueing over nothing that I mind. This kind of stuff was typical. At some point there was a lengthy discussion which prevented rolling out a rather importa…

> it was allegedly "dangerous" because it "could loop infinitely". Well, ehh, that's the case with any lop innit? That's kind of how they work? No? You can clearly have loops that exit. Loops that can be infinite should only be so where you really want that to happen in those cases / are fine with it. "Infinitely until the user does X with no timeout" is a fair one.

Every while/for loop is essentially "keep doing this until I tell you to stop". You can argue what the best way to "tell to stop" is, but that doesn't really change the concept. No one could even articulate under which conditions this could happen. Cosmic rays altering memory? That's the kind of stuff we're talking about here. For a PHP webapp. I don't recall the exact specifics but we spent a long time on that stupid loop and no one even had any alternative. Eventually the loop committed.

The thing is, if I had written the conditions in the loop body it would have been fine with no comments. People were tripped over the "while (true)" and commented on that. That's okay, I can just change that, no problem. But the kept analysing and thinking about it to the subatomic detail. That as really the problem: small things tended to escalate to "a big thing" even when it was just a small thing. I don't know what it was – a kind tunnel vision? I don't know.

I once carefully suggested to paint a wall in a brighter colour during a HOA meeting. People objected. Fine, not a big deal and just a suggestion, I'm okay with the grey it was. They kept discussing it. I back-pedalled harder. People started to suggest we need to form a committee. I started to run. They asked if I wanted to join the committee "as I had brought up the issue". I overtook Usain Bolt. I heard they had several meetings. They choose the same grey it was before (or something so similar I couldn't spot the difference). Not my favourite colour or what I would have chosen, but it's fine.

Post reply on HN