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

191–200 of 302 posts

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

#191

Post by mkrishnan »

noname120 wrote: post #114
mkrishnan wrote: post #107

critique is a worst tool I have ever used. example 1: - a engineer caused multi-million dollar loss in ad revenue due to change which passed review with flying colors. and he is the only engineer whose name ever put in a post mortem document. - same engineer few months before this incident forced a junior engineer to waste 2 months by making him to stupid nit picks. after a month of addressing nit picks, the junior e…

I don't see how examples 1 and 2 are related to Critique. Then example 3 exhibits a tenuous link to the topic but it's unclear how exactly this problem is specific to Critique.

Critique enabled such an outcome. Critique is designed to provide control to dominating individuals in the team to do what ever they want. Critique is super lethal when team leadership is very weak.

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

#192

Post by aerzen »

SpaceManNabs wrote: post #146
tgsovlerkhgsel wrote: post #80

Earlier quoted context omitted.

Bazel is a build system, not a code review system. Entirely different thing.

You should open the article. The very first content in the article is someone praising critique and also saying they wont use bazel.

It's not easy to setup and has a steep learning curve. But it's great to use afterwards.

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

#193

Post by withinboredom »

ravenstine wrote: post #169
withinboredom wrote: post #95

Earlier quoted context omitted.

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

Kind of off topic but I'm curious what it is about a foreach that you think would do a better job than a syntactical for-loop. In my experience, for-loops are almost always the better choice, unless the implementation of "foreach" is done in a way that is functional and recursive. Foreach in the usual sense (like the method in JavaScript) provides too little control to the callee and is only really of benefit if one…

> what it is about a foreach that you think would do a better job than a syntactical for-loop

It depends on the language. For example, an optimizing compiler can optimize the shit out of a for-each loop, but can only go so far in a for-loop. For example, in a for-loop, you can "skip around" the indices, and even go backward after going forward, while in a for-each loop, it will always go one-by-one, and the compiler can rely on that fact. Even the CPU can probably optimize it better since the data access will be more predictable.

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

#196

Post by bluGill »

zeroCalories wrote: post #185
merb wrote: post #173

Earlier quoted context omitted.

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.

Then please write one.

Note that the important part is a good way to mark all those false positives that is simple and doesn't go to far. Just because I want a bad spelling in one place doesn't mean I want it everywhere. As a result I'm going to predict that your tool either results in too much boilerplate needed to suppress all the false positives, or your tool lets pass a lot of things that shouldn't. But that might be just that I don't have good ideas: if you create a good tool for this I'm willing to be proven wrong.

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

#197

Post by dontreact »

seanmcdirmid wrote: post #165
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.

you can mark your comment (in text) as a NIT and then unmark "Action required."

Yeah… but people sometimes dont do this

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

#198

Post by bluGill »

colonCapitalDee wrote: post #183
zeroonetwothree wrote: post #141

Earlier quoted context omitted.

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

Either that or it makes it really hard to find the person who understands the code you just changed to verify you didn't miss anything. I don't work for Facebook, but I'm often the person who does large scale refactorings - since I know I'm human I want an expert to verify those that look at all scary.

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

#199

Post by k_dumez »

joined a startup recently whose founding team was entirely from FB (who used Phabricator internally), and Google (who used Critique internally).

They missed those code review tools so much they just rebuilt it themselves lol.

I was initially skeptical but after using it for like 3 or 4 days it's going to be hard to go back to vanilla git tbh

https://graphite.dev/

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

#200

Post by mstachowiak »

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 that developers enjoy have been included in our first release... - A focus on only the latest changes - A familiar, side-by-side diffing interface - show 'diff from the last review' by default - Tight integration with other tooling - 'Action set' tracking - we allow you to pinpoint and assign line-level issues to relevant team members and track who's turn it is to act - Satisfying gamification - plenty of buttons that go green and even some fun visual rewards for merging

Additionally, we've layered in... - A beautiful, modern UX that provides light or dark mode - Comments that never become outdated and reposition/evolve with the review - Smart version tracking that handles rebases, merges, and force-pushes gracefully - Progress tracking that allows you to see what each participant has left to complete down to the file revision level. - A real focus on trying to get turn tracking right

We're just getting started and have a ton of ideas we can't wait to layer on. If anyone is up for giving it a try, we're actively seeking feedback. If you mention 'Hacker News' in the waitlist form we'll let you in right away.

Post reply on HN