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

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

#271

Post by Tyr42 » Tue, Dec 05, 2023, 3:32 AM UTC

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.

At least with the AI assist it's easy to one click accept the name change and be done with it.

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

#272

Post by MarkSweep » Tue, Dec 05, 2023, 4:09 AM UTC

hyggetrold wrote: post #90

I think it's funny that devs will go through all kinds of untold suffering when it comes to code reviews...stacked pull requests, waiting days (or more!) for a review, all kinds of nitpicky BS that is a lot of time and effort to rework by the time you have the code all written. But suggest to people that they pair program and that it's a real-time review that obviates the need for formal async review and folks want t…

Stacked pull requests are often a solution to, not that cause of, untold suffering. They let you put blinders on your reviewers to they don’t startle about unrelated changes.

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

#273

Post by mgraczyk » Tue, Dec 05, 2023, 4:16 AM UTC

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…

I fought with the review admins about this crap a lot when I was at Google. When you're working toward readability, code that isn't related to change is not in scope. The reviewer can comment on it but you're not expected to change it. That is in the official "rules".

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

#274

Post by Sandworm5639 » Tue, Dec 05, 2023, 4:52 AM UTC

> A CL (or Pull Request) is created in Google’s in-house code editor Cider

Since the article is pretty accurate about everything else, I just want to point out that this is(was?) just one way of creating a CL. I was using IDEA and was mostly creating CLs using a command line.

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

#275

Post by yeukhon » Tue, Dec 05, 2023, 5:54 AM UTC

The one thing I always enjoy looking at Google internal tools is that many of their tool designs remain pretty basic. They look like the old Gmail settings page/old style html page, if you remember.

Very much just tables, div no fancy icons, no fancy fonts. Not a lot of images to render. I like that.

Facebook’s on the other hand, I wouldn’t say fancier, they have a consistent internal design UI components, I can tell you they look less “mature” than Google’s. I really don’t have a word for that but just think like “bootstrap” feel.

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

#276

Post by Xeamek » Tue, Dec 05, 2023, 6:15 AM UTC

hyggetrold wrote: post #106

LeafItAlone wrote: post #94

Earlier quoted context omitted.

I wonder if there are studies on the effectiveness of pair programming vs code review and bugs caught. Anecdotally, I catch more bugs doing code review than pairing. When pair programming, we naturally start to think similar thoughts, so it doesn’t get a great real second set of fresh eyes. Even when I have pair, I like to do an alone-time code review and things almost always pop up.

> I wonder if there are studies on the effectiveness of pair programming vs code review and bugs caught. Whenever folks ask me this, I want to turn it around: are there studies on the effectiveness of working solo vs pair programming? > Even when I have pair, I like to do an alone-time code review and things almost always pop up. Agree - even with code that's been mostly paired on it can be helpful to let it sit and…

>Whenever folks ask me this, I want to turn it around: are there studies on the effectiveness of working solo vs pair programming?

Pair programming inherently halves your potential productivity, as you have 2 people doing 1 work. Additionally, you introduce additional friction because of communication between the two programmers, and finally, you still (usually) need a review of the final code, even if we assume it's better then in 'solo' scenario. So for the pair programming to be objectively effective, it would need to provide over 100% of productivity boost minimum, because thats how much it takes away. That's a huge ask, and it's on advocates of it to prove that. Normal process in comparison, because of having inherently twice the working power, can waste up to 50% of person's productivity and still be ahead of pair programming starting position. The 'math' is simplified ofcourse, but the burden of proof is on pro-pair people by default, not the other way around.

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

#277

Post by withinboredom » Tue, Dec 05, 2023, 8:07 AM UTC

redditor98654 wrote: post #268

withinboredom wrote: post #257

Earlier quoted context omitted.

Eventually, sure. I'm also starting to think this is how the Paradox of Tolerance happens. I'm intolerant of intolerance these days. I'm much more tolerant of people having their way of doing things than I was just 10 years ago. However, as soon as one person comes along and says "It MUST be this way because QUALITY!" I go into intolerance mode and don't tolerate it. Now, if you can give me a reason grounded in compu…

I see. I want to try and understand this because I am also trying to get better at code reviews and not come across as a dogmatic person. I have spent almost 15 years in mostly AWS and I want to keep myself in check and make sure people don't take my suggestions as the vague "quality" as you so mention just because of my seniority. Here is the most recent PR I did for a relatively young person in my org. Part of the…

There are three parts to every code review:

1. Code style: such as formatting and when to use certain things (non-negotiable and you really should automate that).

2. Working code: does the PR have a description/ticket and does the code do what it promises to do? Can we refactor anything to make it better?

3. Conventions: does the PR have tests when necessary, are there negative and positive tests? Does it pass those tests? Is there anything against the conventions? If so, bring it up with the dev and find out why, outside of the PR. Maybe they didn't know the convention or there is some legitimate reason for it. Don't be an ass and call them out for it on the PR in front of the whole team, give them the benefit of the doubt and if someone else comes along and calls them out, you can present a united front vs. forcing them to defend themselves all alone.

In this particular case:

Suggesting a library should have been done before the code was even written. At this point, I wouldn't even suggest using it without some homework for the reviewer and look at the implementation in the library.

If the library implementation covers cases not found in the code I was reviewing, I would suggest looking at that library for some inspiration and/or switching to it. I would emphasize switching to it if it is far and away from the library code; as time could be better spent elsewhere. If the new implementation looks better than the library one, I might even suggest refactoring the entire codebase to use the new implementation instead of the library and/or spending some time to open a PR with the original library and improve it.

The work has already been done. Try to capitalize on it, instead of dismissing it.

Seriously though, look at the library implementation before moving forward. I've seen some very popular libraries and frameworks be very poorly implemented in some areas but because "it's popular" people seem to think it is better. That's not always the case.

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

#278

Post by crotchfire » Tue, Dec 05, 2023, 9:27 AM UTC

cmrdporcupine wrote: post #143

sparcpile wrote: post #135

Earlier quoted context omitted.

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…

I assume that Critque is Grerrit with a bunch of Google-specific changes. Not even close. I have another comment where I get into some details, but, no, three's no overlap beyond the fact that Gerrit pulled some UI and workflow things from Critique (and Mondrian before that, the tool that predated Critique)

I dunno. I use gerrit frequently and nothing in this article surprised me. Aside from "ML-powered woo woo" I've seen and used everything bragged about in this article.

Gerrit is awesome. I will never, ever go back to github.

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

#279

Post by crotchfire » Tue, Dec 05, 2023, 9:30 AM UTC

cmrdporcupine wrote: post #144

nappy-doo wrote: post #56

Earlier quoted context omitted.

Gerrit is pretty crap compared to critique. It has a workflow that works for Android, but critique is really much better.

The essential thing is that Gerrit is a swiss army knife review tool for git , whereas Critique is able to be consistent and fluid because it only has to worry about working with the standard workflow in Piper/CitC (and now fig I guess) I agree Critique is much nicer, but mostly because it's more consistent and doesn't have to deal with all the oddities of git.

Can you be more specific?

Gerrit does in fact impose a particular workflow: each commit is the atomic unit of review.

This, BTW, is a beautiful thing. Most of the idiocy of github is trying to have multi-commit pull requests. Then they had to bodge on that "suggested changes" nonsense instead of having proper dependency tracking between pull requests.

When each commit is the unit of review, you get pull request dependencies and recursive pull requests automatically. Instead of suggesting changes you can simply create a commit with your suggestion which has the reviewed commit as its parent. It's so simple yet so much more powerful.

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

#280

Post by mike_hearn » Tue, Dec 05, 2023, 9:45 AM UTC

mplanchard wrote: post #255

mstachowiak wrote: post #245

Earlier quoted context omitted.

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?

I was just wondering! I'm on Linux, so I wasn't sure on reading the web page whether it was something I'd be able to run, and the screenshots looked more desktop-app than web-app. We use GitLab at work, so I wouldn't be able to use it there, but I use GitHub and sourcehut for some personal and open source stuff. Code review is one of the few things I don't do in emacs, so there remains room for other tools :)

One thing you could try is using a JetBrains IDE. They can do side-by-side diffs, static analysis in the diff view, you can edit directly from the diff viewer and of course you get full navigation and comprehension tools. When I left Google I spent some time trying to use GitHub's code review tools, but they are extremely basic. In recent years I found that with a custom git workflow I could use the IDE as a code review tool and it worked much better than any web based thing.

The trick is to use git commits as the review comments. As in, you actually add // FIXME comments inline on someone else's branch. They then add another commit on top to remove them. Once you're done, you can either squash and merge or just merge. This is nice because it avoids "nit" comments. If you dislike a name someone picked, you just go change it directly yourself and avoid a round-trip, so it reduces exhaustion and grants a notion of ownership to the reviewer.

If you need discussion that isn't going to result in code changes (about design for instance) you do it on the relevant ticket. This also has the advantage that managers who stay away from code review tools still feel like they understand progress.

It helps to use a specific way of working with git to do this, and to have it integrated into your other tools. I use gitolite combined with YouTrack and TeamCity to implement this workflow, along with a kernel-like ownership tree, but it works well enough at least in a small team (I never got to try it in a big team).