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

Post by KptMarchewa » Mon, Dec 04, 2023, 6:06 PM UTC

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?

Post by layer8 » Mon, Dec 04, 2023, 6:11 PM UTC

TheBlight wrote: post #8

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.

Excessive subjective feedback can be soul-crushing.

Post by singron » Mon, Dec 04, 2023, 6:12 PM UTC

corethree wrote: post #96

Google could easily release a GitHub competitor. But they don't. Similar to how they invented and failed to popularize the LLM. It's like the company is run by both geniuses and clowns.

Post by arp242 » Mon, Dec 04, 2023, 6:14 PM UTC

ryandrake wrote: post #77

arp242 wrote: post #60

Earlier quoted context omitted.

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

I'd wonder what the rest of that API surface looked like. If the rest of the functions were longDescriptiveCamelCase() and then you tried to slip in mkdir(), then I'd say yes, "stylistic consistency" was gained during that unnecessarily difficult back-and-forth. If the other functions were chdir(), remove(), rmdir() and so on, and somehow the reviewers picked your code change as the time to change it all, then yea, w…

  while (true) {
    if (someCond()) break;
    if (otherCond()) break;
    [..]
  }

  /*! jslint: keyword1 evil keyword2 */
  /*! MIT license blah blah */
  minified_js()...

Post by hyperpape » Mon, Dec 04, 2023, 6:21 PM UTC

tuetuopay wrote: post #85

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 get the same feeling when switching between Gitlab and Github. The latter is a joke of usability and discoverability. Tiny details like thread filtering, thread replies not appearing as separate comments, "show changed lines" when a new commit modifies the subject of a comment, etc. Those add up to make gitlab much more usable.