A study of Google's code review tooling (Critique) • Hacker News Board

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

#181

Post by runlevel1 » Mon, Dec 04, 2023, 8:09 PM UTC

wubrr wrote: post #54

acscott wrote: post #15

Earlier quoted context omitted.

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.

That seems like it risks creating conflict out of what's often just a misunderstanding.

Assuming it's a corporate environment (it's fuzzier in the open source bazaar):

If it's the first time or I don't really know the reviewer, I ask them to hop on a call to discuss (usually to walk me through) their feedback and I go in with an open mind. That gives me the opportunity to find out if I'm missing some context, can see how reasonable they are, and can get clarification of what they actually care about versus FYIs/suggestions. As they go, if it isn't clear, I just ask them if something is a soft opinion or hard opinion.

If everything is a hard opinion and they don't seem reasonable, I reach out to someone else (ideally a team lead or peer on their team) over a private channel for a 2nd opinion. If they also think it's unimportant stuff, I ask them to add their own comments to the PR. Give it a reasonable amount of time and they'll either have reached a consensus or you can roll the side you agree with.

If it's an issue again later and they seem reasonable, respectfully push back. If they seem unreasonable, skip right to DMing their lead for a 2nd opinion.

If it keeps being in issue, then some frank conversations need to happen. Something I've noticed about folks who steadfastly focus on minor stylistic nits in CRs is they (1) tend to be cargo culting them without understanding the why behind them and (2) they're usually missing the forest (actual bugs in logic) for the trees.

Most people are pretty reasonable when they don't feel like they're under attack, so in my experience it's usually possible to resolve these things without dragging it out. Of course, if you're at a company with a lot of disfunction, well... I can understand why what I've written above won't work.

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

#182

Post by throwawaaarrgh » Mon, Dec 04, 2023, 8:14 PM UTC

cgearhart wrote: post #62

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…

This kind of greedy optimization approach is fine if all you’re just trying to maximize velocity, but it starts showing cracks on a long enough timeline in sufficiently large or complex projects. Speed without alignment on direction leads to ugly, tangled messes. As with most quality issues, the key is to try and surface quality problems as early as possible. Finding a bug in prod is worse than finding it in PR is wo…

> it starts showing cracks on a long enough timeline in sufficiently large or complex projects

Show me a project which this doesn't happen to on a long timeline and I'll show you a project with no users and perfect programmers.

Entropy is unavoidable. It will eventually degrade despite your best efforts and in the meantime you aren't benefiting from a working feature. We should try to delay software entropy as much as practical, but not at the expense of shipping.

Basically there is a balance to strike. I don't have a formula for it, but I do know that merging often ends up being more valuable than merging later just to ensure clean code. Working features are more valuable than a mess. A mess sucks, so we should try to avoid it, but the world runs on messes, not clean code.

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

#183

Post by colonCapitalDee » Mon, Dec 04, 2023, 8:20 PM UTC

zeroonetwothree wrote: post #141

joatmon-snoo wrote: post #133

Earlier quoted context omitted.

As someone who left in 2021: - OWNERS is absolutely a necessary and important thing, and yes it sucked when it made finding an approver hard, but the point of OWNERS was to optimize for _local_ ownership. (For non-Google folks: think CODEOWNERS files, but hierarchical/recursive, so approvers in /OWNERS, a/OWNERS, a/b/OWNERS, and a/b/c/OWNERS can approve changes anywhere in a/b/c/...) - I joined in 2017 and it never t…

At Facebook there is no OWNERS so if you make an API change you can just update all the code yourself at once. It makes it much easier to rapidly iterate and improve shared libraries.

Yes, and it also makes it much easier for random people to break your code. It's a trade-off

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

#184

Post by dvirsky » Mon, Dec 04, 2023, 8:23 PM UTC

sparcpile wrote: post #135

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.

It looks like Critque is a branch of Gerrit. The user interface is similar. I assume that Critque is Grerrit with a bunch of Google-specific changes. Gerrit itself is an interesting review tool. It uses Git references to manage the review changeset before it is merged into the parent branch. I used it on a project that used Redmine for issue tracking and Gerrit for the git repo and review tool. It took a bit to get u…

> It looks like Critque is a branch of Gerrit

IIRC Gerrit is an open source re-implementation from scratch of Critique.

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

#185

Post by zeroCalories » Mon, Dec 04, 2023, 8:24 PM UTC

merb wrote: post #173

zeroCalories wrote: post #166

Earlier quoted context omitted.

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

Doesn't seem hard to me? You could easily run a spell checker on identifiers and comments. It will produce a lot of false-positives, but that can be solved by making the changes optional or using an allow list.

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

#186

Post by runlevel1 » Mon, Dec 04, 2023, 8:29 PM UTC

faster wrote: post #63

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.

I struggle with this. I work with devs who think that a function with 3 side effects is fine, and suggestions that it would be more maintainable if it did one thing (and even more offensive, that the function name should be a clue to what the function does) are often met with hostility. In the end they're angry and resistant and I'm frustrated. Sometimes I just approve it, write a technical debt ticket, and move on.

Anger and frustration is obviously not an appropriate response to constructive feedback offered in earnest, but do you have any insight into why they respond that way? Like naïveté as to why it's usually a bad idea to do that, them misinterpreting your intent, or redirected aggression from another source of frustration (like workload, deadlines, etc.)?

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

#187

Post by BytesAndGears » Mon, Dec 04, 2023, 8:32 PM UTC

withinboredom wrote: post #137

notnullorvoid wrote: post #125

Earlier quoted context omitted.

They are not suggesting hazing. I don't doubt that you experienced hazing. Being vigilant with new hires to assure assignment on code quality and design is not hazing though. If someone has legitimate concerns about the design decisions made, then they should voice them. However if they are refusing to adhere to guidelines, simply because they dislike the approach then that's being overly problematic.

It’s literally the definition of hazing. But instead of being asked to jump in a pool, naked, while snowing, you are asked to build things a new hire has no business building. Then nit-picked for not knowing things. Literally set up for failure. A better solution is to actually sit with them while they build a feature, show them around the code, and answer questions. You know, treat them like a team member instead of…

I definitely didn’t mean it like hazing.

I don’t see what’s so wrong about saying “you used inheritance for this relationship, but we have a pattern of keeping classes like this separate, since this system tends to change frequently. Please organize this like xyz module instead.”

Just a random example of something that a new person might do who is unfamiliar with xyz module and the complications there.

I agree that it’s good to mentor someone new, but honestly I think they still make most decisions themselves, and sometimes those decisions don’t match established patterns that they don’t know about. Ideally you notice sooner than a PR but I also think that people get busy and it’s ok to not have time to monitor everything a new person does. So sometimes it comes down to the code review to notice.

It’s not derogatory or hurtful, literally at all, it’s just pointing on that they did something in a way that goes against established patterns, and it’s teaching them what those patterns are.

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

#188

Post by withinboredom » Mon, Dec 04, 2023, 8:32 PM UTC

yjftsjthsd-h wrote: post #176

withinboredom wrote: post #102

Earlier quoted context omitted.

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…

I hear you, and I agree, somewhat. Hell, my wife calling me is probably more important to me than a code review.

That being said, I do make doing code reviews _the_most_important_thing_, except that I make it utterly transparent when I check for code to review (first thing in the morning and after lunch). I also make it clear that I'm only spending 30 minutes (max) on the review. If I can't review it in less than 30 minutes, I'm leaving a comment that the PR is too big and needs to be broken up and then not reviewing it (maybe someone else on the team is willing to review it).

That's my policy and the team agrees. Other team members have their policies as well (some won't review until they are also waiting for a review, some simply don't want to do reviews at all and only begrudgingly review and some are very similar to mine).

I think it is ultimately a conversation the team must have to have predictable releases and deployments. If you are doing nothing but code reviews for a couple of days before releasing ... you're going to have some fun integration issues and need quite a bit of manual testing.

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

#189

Post by joshuamorton » Mon, Dec 04, 2023, 8:36 PM UTC

arp242 wrote: post #180

IanCal wrote: post #164

Earlier quoted context omitted.

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

I can prove, pretty trivially that

    for i in range(10):

Will halt.

Slightly less trivially, you can prove that

    while true:
        if x:
            break
        else:
            break

Will halt.

But once the if conditions aren't exhaustive, it becomes difficult, perhaps impossible, to know that you won't end up in a weird unhandled state.

Preferring and even insisting upon code that is not only possible, but easy, to reason about is good. Or iow it should be incumbent on you to prove the loop halts, not on the reviewers to prove it doesn't.

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

#190

Post by withinboredom » Mon, Dec 04, 2023, 8:39 PM UTC

BytesAndGears wrote: post #187

withinboredom wrote: post #137

Earlier quoted context omitted.

It’s literally the definition of hazing. But instead of being asked to jump in a pool, naked, while snowing, you are asked to build things a new hire has no business building. Then nit-picked for not knowing things. Literally set up for failure. A better solution is to actually sit with them while they build a feature, show them around the code, and answer questions. You know, treat them like a team member instead of…

I definitely didn’t mean it like hazing. I don’t see what’s so wrong about saying “you used inheritance for this relationship, but we have a pattern of keeping classes like this separate, since this system tends to change frequently. Please organize this like xyz module instead.” Just a random example of something that a new person might do who is unfamiliar with xyz module and the complications there. I agree that i…

My response would be a simple "Why does code changing frequently prevent inheritance from being used?" if I got a comment like that. Granted, I don't like inheritance, so I have nearly 1000 arguments on reasons not to use it that has nothing to do with code changing frequently, so ... this is probably a bad example for me, personally.

But seriously, I'd ask why, and why again. I'm very much against cargo culting, and I will refuse to make changes in my PR if it is cargo culting. You're welcome to open a PR to my PR with changes if you feel strongly about it though.

Maybe this makes me hard to work with, but so far, I feel like it has led to better code and a higher velocity, everywhere I've worked.