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

241–250 of 302 posts

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

#241

Post by compiler-guy »

tunesmith wrote: post #206
ayberk wrote: post #172

Earlier quoted context omitted.

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…

Getting reviews from multiple people that disagree on style is definitely an org problem that sucks to be in the middle of. However, I'd say that getting nit comments on code that surrounds your changes, but that you didn't change, is still fair game. It's part of the leave your campsite cleaner than you found it. It depends on the culture though - if it's suggested in the manner of "since you're here, here's an oppo…

This can add cost and latency to the change author and distracts them from the project at hand. And because it costs the reviewer basically nothing to say, "While you are in there....", it slows the original author's work.

Even worse, if there are enough of these minor changes, some other reviewer may take issue with their "small, focused changes" preference, and ask you to split it.

This is why Google has the "It doesn't have to be perfect; it just needs to be better" standard: So reviewers can't impose undue costs on authors.

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

#242

Post by TeMPOraL »

tikhonj wrote: post #30
bheadmaster wrote: post #23

Earlier quoted context omitted.

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

Are you saying there is no such thing as clearer or less clear writing in natural languages or programming languages? It's 100% subjective and depends entirely on the reader?

Some thing are objectively bad. But a lot is up to, if not subjective preferences, then at least situational ones. With modern programming languages, we're operating close to Pareto frontier in terms of expressiveness - there's just no way to make a piece of plaintext code more readable for some readers, without sacrificing readability for other readers with different goals.

Things like "lots of small functions vs. few large functions", or "exceptions vs. sum type return value", are such cases - they seem subjective, but they're less about preferences, and more about the kind of work a given reader is doing. Either choice is better than the other for some kind of work. We can't improve on this until we move past working directly on plaintext, single-source-of-truth codebases.

Plaintext is fine. Single source of truth is obviously needed. The problem is with insistence on only ever working directly on it, which leads to a futile attempt at inventing styles and languages that would express every cross-cutting concern and needs of every job in a clear and readable fashion. It's just not possible.

(And yes, I believe the bleeding edge of programming language development is effectively just spinning the wheels now - adding increasingly complex abstract math to programming languages isn't going to help square the circle.)

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

#243

Post by mplanchard »

mstachowiak wrote: post #200

Shameless plug - I'm one of the creators of GitContext ( https://gitcontext.com ), a code review tool which has drawn much inspiration from Critique and others, and would love feedback from anyone who's interested in kicking the tires. We just launched in private alpha. We're putting a maniacal focus into the user experience of code reviews, which we feel is overlooked by most tools. Many of the features of Critique…

Is this a desktop app? If so, is it cross-platform? I couldn’t find this info anywhere on your site.

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

#244

Post by joshuamorton »

arp242 wrote: post #223
joshuamorton wrote: post #189

Earlier quoted context omitted.

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

You can prove that a different construct from a different language can do a different thing because it's a different construct from a different language. > it should be incumbent on you to prove the loop halts This is a nice one-liner platitude, but also completely unworkable.

> You can prove that a different construct from a different language can do a different thing because it's a different construct from a different language.

This is the same construct, it's just using python syntax instead of c/c++ syntax. There aren't functional differences between the examples.

[Edit: To elaborate, your initial example translated to python is

    while(true):
        if x:
            break
        if y:
            break
and there is no way to verify that `x || y` is always true. If you can in fact show that it's always true, you can probably restructure the code to both convey that invariant (use an exhaustive if/else if check) that is better supported by linters and gives you warnings when you miss something.

Writing code in a way that makes it more difficult for both humans and machines to divine your intent should be questioned in review! Perhaps it isn't possible to verify that (but that's also concerning), or perhaps you have some good justification for doing the unusual thing, but then that justification should also go in the code as a comment to give future folks context on why you have a strange and misleading pattern.]

> This is a nice one-liner platitude, but also completely unworkable.

Perhaps, in some cases (yes, we can't always follow NASA's coding standards), but ensuring that most of the time invariants are locally verifiable is possible, that's what typecheckers are, and it's not like Java or Rust are unworkable languages to write code in.

Just because we can't statically encode "this loop halts" as a type, doesn't mean that we shouldn't make that fact clear and verifiable by a human. And "it is incumbent on the person writing the code to justify why it needs an exception from " is exactly what code review is for. It's not that hard at all. It's how I review code, and how I expect my code to be reviewed.

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

#245

Post by mstachowiak »

mplanchard wrote: post #243
mstachowiak wrote: post #200

Shameless plug - I'm one of the creators of GitContext ( https://gitcontext.com ), a code review tool which has drawn much inspiration from Critique and others, and would love feedback from anyone who's interested in kicking the tires. We just launched in private alpha. We're putting a maniacal focus into the user experience of code reviews, which we feel is overlooked by most tools. Many of the features of Critique…

Is this a desktop app? If so, is it cross-platform? I couldn’t find this info anywhere on your site.

Sorry for the confusion. It's currently only offered as a web application and only works with GitHub. We are working to expand beyond these limitations based on customer needs / interest. I assume your interest is in a desktop application?

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

#246

Post by stavros »

codeapprove wrote: post #41
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…

This looks like a good tool, and I was tempted to try it, but it costs twice as much as GitHub itself, and I'm not sure it would give us twice as much value.

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

#247

Post by stavros »

mstachowiak wrote: post #200

Shameless plug - I'm one of the creators of GitContext ( https://gitcontext.com ), a code review tool which has drawn much inspiration from Critique and others, and would love feedback from anyone who's interested in kicking the tires. We just launched in private alpha. We're putting a maniacal focus into the user experience of code reviews, which we feel is overlooked by most tools. Many of the features of Critique…

I'll say what I said downthread to someone who made something similar: This costs more than twice as much as GitHub, does it provide twice the value?

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

#248

Post by mplanchard »

froh wrote: post #218
yohannparis wrote: post #204

Earlier quoted context omitted.

Yes, I have been "suggesting" a lot of one liners, typo, rephrasing, or just simple clean up of code with it. Make it a breeze, it's like playing tidy-up without having to branch out or bother much the author.

I feel stupid. How do I do this?

When writing an inline comment, there’s a button to make a suggestion. It inserts a markdown code fence with the existing code on the line(s), which you can edit. When you submit the comment, it shows up with diff highlighting and can be applied by the author with one click.

Essentially the same thing exists in both GitHub and GitLab

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

#249

Post by randall »

stavros wrote: post #247
mstachowiak wrote: post #200

Shameless plug - I'm one of the creators of GitContext ( https://gitcontext.com ), a code review tool which has drawn much inspiration from Critique and others, and would love feedback from anyone who's interested in kicking the tires. We just launched in private alpha. We're putting a maniacal focus into the user experience of code reviews, which we feel is overlooked by most tools. Many of the features of Critique…

I'll say what I said downthread to someone who made something similar: This costs more than twice as much as GitHub, does it provide twice the value?

GitHub's code review is pretty mediocre imo... just left Meta and I miss phabricator. I'm interested to see new stuff, hope I can get off the waitlist!

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

#250

Post by stavros »

randall wrote: post #249
stavros wrote: post #247

Earlier quoted context omitted.

I'll say what I said downthread to someone who made something similar: This costs more than twice as much as GitHub, does it provide twice the value?

GitHub's code review is pretty mediocre imo... just left Meta and I miss phabricator. I'm interested to see new stuff, hope I can get off the waitlist!

I definitely agree there, I just don't know if a tool that adds a feature is worth twice as much as the tool that the feature is being added to.
Post reply on HN