16 ms·

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

291 points by vinnyglennon 3y ago

manueljose46567 3y ago
[dead]
williamsherron 3y ago
[dead]
SuzetteClayton 3y ago
[dead]
seydor 3y ago
[flagged]
dontreact 3y ago
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.
- shadowgovt 3y ago
  It can be tricky to find a balance point here. At Google scale (in time-of-maintenance, not space... Code can last for years and will be worked on by multiple engineers disconnected from the original project), the hard problem of naming things becomes real. I find that it's useful to keep context. Even though one can never predict with certainty, sometimes you can be real confident that some code is prototype that will be thrown away in six months. And there's a big difference between the code that makes up an API layer and the code that implements a feature constrained to one module.
  - arp242 3y ago
    At not-Google-scale code also often lasts for years; perhaps even more so since there's typically a lot fewer people maintaining it. I think the key thing is to ask yourself "is this really objectively better yes/no?" before commenting. Not that you can never comment if the answer to that is "no", but quite a lot of the time when the answer is "no" it doesn't really matter and it's just "I would have done it slightly different, but both are fine".
    - shadowgovt 3y ago
      Good point. Making renames a suggestion, not gating (unless it contradicts the name in some design doc somewhere that another team is relying on not to move) may be the balance point there.
      - arp242 3y ago
        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 pointed out that "dir" was a needless abbreviation so it became createDirectory(). Then yet someone else pointed out that it's actually recursive, and a discussion on the merits of createDirectoryRecursively() vs. createRecursiveDirectory() was inflicted on everyone. In-between there was a side-quest about directory vs. folder. Just fucking stick a fork in my eye already. Was anything of any objective value gained? I'm having a hard time seeing it. Well, it makes for a slightly amusing anecdote so there's that.
        ryandrake 3y ago
        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, what a waste.
- TheBlight 3y ago
  Excessive subjective feedback can be soul-crushing.
  - acscott 3y ago
    Agreed; after > 20 years of coding successfully, got hit by a storm of subjective feedback; it totally ruined any joy in development
    - TheBlight 3y ago
      It didn't used to be this way. A code review used to mostly function as a quick sanity check. Now it's basically code by committee.
    - wubrr 3y ago
      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.
      - runlevel1 3y ago
        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.
- jeffbee 3y ago
  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.
  - dontreact 3y ago
    Im not saying it’s always nitpicking. I’m saying that sometimes it is.
  - bheadmaster 3y ago
    "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 readable".
    - tikhonj 3y ago
      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?
      - bheadmaster 3y ago
        Of course, it's not 100% subjective - rarely anything in the world is 100% anything. But I think that most people consider their personal preferences to be better and more readable just because they're used to them, so I tend to take the opposite attitude as the starting point. There were more than a few situations where I've had a coworker tell me "just read the code, it's very readable", only to spend the next two weeks just trying to figure out how it works. Sure, once you figure out how it works and it "clicks", it's no longer (that) unreadable, but the fact that I have to spend so much time reading the codebase in the first place made me convinced that personal familiarity is a great part of what "readable" means.
- seanmcdirmid 3y ago
  you can mark your comment (in text) as a NIT and then unmark "Action required."
  - dontreact 3y ago
    Yeah… but people sometimes dont do this
- ayberk 3y ago
  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++ readability was a much, much better experience. All the comments were about actually making the code better, eg, "use THIS_MACRO() instead of THAT_MACRO(), because go/...". I guess I think it's much more about the reviewer, and based on my anecdotal experience, the language :)
  - tunesmith 3y ago
    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 opportunity for how to improve this area of the code", that's better then acting like you made a mistake in failing to change it.
    - xKingfisher 3y ago
      I try to avoid nits totally unrelated to the changes at hand, since on a subconscious level they may discourage people from even wanting to touch older/less loved files at all. The critical exception being avoiding issues due to path dependence. E.g while a change is "correct" is doing X poorly because of surrounding issue Y. So we should fix Y now instead of building atop it.
    - eichin 3y ago
      Something I find helps with this in particular is only allowing style comments with citations to an actual style guide item. (I talk about domain-specific style guides as "crystallized arguments" - we agreed on this and wrote it down, not because it's necessarily right (though it probably is) but that we really wanted to stop wasting time arguing about these particular things.)
- egl2021 3y ago
  Different languages had different pools of readability reviewers, so the expectations varied, but readability reviews were generally constructive and helpful. I was thrilled to have Ian Taylor review my go code. The non-readability reviewers were usually on your team, so there was social pressure in both directions. You wanted to learn and conform to the team's norms, and the reviewer couldn't be a total jerk about their comments. Everyone was generally on their best behavior.
- Tyr42 3y ago
  At least with the AI assist it's easy to one click accept the name change and be done with it.
- chii 3y ago
  > Sometimes it’s not worth arguing over small details like variable naming but naming is quite important. May be it's not that this is a nitpick, but that previous review tools prevent the fruitful discussion of names.
shadowgovt 3y ago
That ML-sidecar for recommending the actual change is a very correct place to put that technology: right in the line where someone is already reviewing the code, so they can confirm the ML suggestion looks correct before adding it. I can't overstate how valuable it is for reviewers to give the actual change they're recommending alongside the description, but it happens too infrequently if we make the reviewer write it every time. Actual code is the cleanest language to communicate ideas, but it's a time-sink and a mental drain to turn the English description into code snippets every time. Automating that is precisely the right place to save time for reviewers.
jdeaton 3y ago
Critique is nice but the ML suggested edits are a waste of attention and cant be disabled.
- shadowgovt 3y ago
  What's the current miss-rate on them? In principle, it seems brilliant, but I can see that value dropping off a cliff if more than half of them are worthless.
  - michaelt 3y ago
    > What's the current miss-rate on them? Depends how good your code is :)
  - hot_gril 3y ago
    Pretty low it seems. Occasionally I get obviously bad suggestions that are easy to ignore.
- mattrb 3y ago
  They can be disabled. Click the settings cog.
- dpbriggs 3y ago
  I disagree. They're pretty good at auto-fixing minor issues and suggesting best practices.
jeffbee 3y ago
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 practiced code review.
- fishnchips 3y ago
  To be fair to GitHub, there has been slow but constant progress over the last few years. I would love it to get at some point to where Critique was when I left Google (almost 10 years ago).
  - charcircuit 3y ago
    I don't see any desire for them to actually improve it. It's been over a decade and the code you are actually reviewing is not even on the page of a PR. You have go to a separate web page to see it.
- codeapprove 3y ago
  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 https://codeapprove.com) to bring a lot of Google's best code review practices to GitHub. It doesn't give you everything Critique did, but I think it brings the same speed, clarity, and focus in a way that's still compatible with the rest of your GitHub workflow.
  - stavros 3y ago
    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.
yx827ha 3y ago
If you want something similar, check out Gerrit: https://www.gerritcodereview.com/ https://www.gerritcodereview.com/ It's open source and used by Android and Chrome.
- jasonlotito 3y ago
  Yeah, it seems like Gerrit with lots of Google-specific stuff. Not surprised. Used Gerrit for 12 years, and loved it.
  - nappy-doo 3y ago
    Gerrit is pretty crap compared to critique. It has a workflow that works for Android, but critique is really much better.
    - cmrdporcupine 3y ago
      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.
      - crotchfire 3y ago
        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.
    - k_dumez 3y ago
tallowen 3y ago
I’m always surprised to see the totality of support for this workflow. My biggest gripes were: - Owners - Readability review (or really the 18month queue to get Java/python readability) Although seemingly innocuous, this made maintaining internal libraries very challenging. There was no way to update every call site of your library in an efficient way. Tools like Rosie were added on top so that you could shard your PRs and shepherd many PRs through a Byzantine approval process. Compared to Facebook, I found it much harder to reach the same levels of productivity given Owners and readability review. I don’t think libraries like React could get developed at Google given how hard it would be to evolve the API surface.
- jeffbee 3y ago
  Google3 is a huge codebase of high quality that moves exceptionally quickly, so it just seems like your priors are getting in the way. Complaining that a large-scale change among tens of thousands of software developers requires a tool (Rosie) is sort of the same thing as all those people who complain that it's too hard to cope with having millions of machines in prod, i.e. the type of people who wash out of the company in a few months and then go on to a career of complaining about it online.
  - marcinzm 3y ago
    The same applies to Facebook which is their comparison and not some random 10 person startup.
    - jeffbee 3y ago
      Except Facebook's codebase is an order of magnitude smaller.
      - tallowen 3y ago
        What makes you say that Facebook's codebase is an order of magnitude smaller?
throwawaaarrgh 3y ago
> 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 written it, get out of the way. If you're saying anything that isn't directly related to a necessary change to the code, get out of the way. When I review, if there's no bugs, and nothing that's going to affect performance, and it passes tests and works, I approve it. If somebody wants mentorship I'll add my thoughts in a comment along with my approval. I don't have this kind of authority, but I would also recommend just not requiring a PR for certain changes. Create a guideline so that changes which a person couldn't improve by reviewing or that have no impact on the working product are just merged. More merging, less useless blockage.
- onlyrealcuzzo 3y ago
  Code review at most companies is an ego thing, for reviewers to feel self-important. It's a way for senior people to pieces of crap to everyone underneath them because that's what they put up with for most of their career - and now they feel like it's their turn to be a piece of crap back and perpetuate the cycle. So many industries run on this exact same cycle. At Google, the company does a good job convincing you that you're in the top x% and lucky to be there. People generally don't really feel the need to crap on people in code review to make themselves feel self-important.
  - sanderjd 3y ago
    I never know whether I've just been fortunate in my career or what, but I've just never experienced anything approaching the kind of unkindness that I frequently see people say is common. I dunno, maybe it's just me, but I'm skeptical. I've always worked with professional colleagues who are trying to figure out how to do their best, and have never once seen people behaving in this childish way.
    - 3y ago
raverbashing 3y ago
Wait... this looks like Gerrit with extra plugins The experience can be nice sure, but Gerrit is not something I'm too enthusiastic about (and yes, Github is better in some aspects, Gerrit is more configurable though and it seems it is smarting tracking "non official" merges)
- compiler-guy 3y ago
  Gerrit is a released version of Google's code review tool called Mondrian--the predecessor to Critique. Critique is basically Gerrit 2.0, although Gerrit has evolved somewhat independently.
  - raverbashing 3y ago
    There was also Ritveld (ugh) before Gerrit And yes, I won't say much about Ritveld besides: ugh
- barrkel 3y ago
  Gerrit is a poor man's Critique and it's not nearly as slick to use in practice.
abtinf 3y ago
Are there any examples of successful organizations, maybe even with SOC2 compliance, that don't have code reviews? From my research, I think Netflix might not have them, but I'm not sure.
- seadan83 3y ago
  Instead of gating at review time, an org could gate at deployment time to achieve that SOC2 compliance. It appears that Netflix does do CR: "Netflix uses a feature branch workflow, where developers create new branches for each feature they're working on. These branches are then reviewed through a pull request process, where other developers can review the code and provide feedback" [1] [1] https://medium.com/@seyhunak/learning-best-practices-from-netflix-tech-stack-ci-cd-pipeline-a25a58f46711 https://medium.com/@seyhunak/learning-best-practices-from-ne...
willsmith72 3y ago
that ui sure is ugly. google really is an engineering company, and i love that, but it's still kind of jarring
- qmarchi 3y ago
  It's definitely looking better than even a year ago. There's a lot of internal UI reworks ongoing to bring things to consistency.
compiler-guy 3y ago
Worth noting that Google engineers had this level of satisfaction long before the mostly useless AI suggestions came on the scene. Those additions are comparatively recent.
- teaearlgraycold 3y ago
  My experience with the AI recommendations was mostly positive as an L4 doing Java
- vinkelhake 3y ago
  I find them generally very helpful. In a majority of cases, it's able to translate my review comments into the exact delta that I had in mind. Source: me, C++ readability reviewer - this means I review code from lots of different people, all over the codebase.
  - andromeduck 3y ago
    It takes way too long though - it's almost always quicker to just do it in cider/subl if there's more than 1 of them.
- summerlight 3y ago
  I think the AI-powered suggestions are often useful, around 30~40% accuracy. The number might seem underwhelming. But when I work on code written in unfamiliar language/frameworks, this saves lots of my time because it gives some hints on how a possible change looks like and which keyword I need to look up for.
SpaceManNabs 3y ago
What is wrong with bazel?
- tgsovlerkhgsel 3y ago
  Bazel is a build system, not a code review system. Entirely different thing.
  - SpaceManNabs 3y ago
    You should open the article. The very first content in the article is someone praising critique and also saying they wont use bazel.
    - tgsovlerkhgsel 3y ago
      Thanks - missed that because it was in a picture. I even went back and CTRL-F'd before I replied but of course that didn't work either...
    - aerzen 3y ago
      It's not easy to setup and has a steep learning curve. But it's great to use afterwards.
codeapprove 3y ago
Shameless but relevant plug: I built CodeApprove (https://codeapprove.com https://codeapprove.com) to bring the best parts of the Critique workflow to GitHub PRs. Obviously there are many things that don't translate (Google doesn't use git, and it has insane CI/CD integration) but CodeApprove gives you the same workflow. You always know which conversations are resolved, which PRs need your attention, etc. Feel free to reach out via email if you're curious.
- mtlynch 3y ago
  I'm a former Google developer and current CodeApprove customer, and I just want to endorse CodeApprove. They're the closest thing I've found to Critique outside of Google. The UI is really straightforward, and Sam (the founder) has been responsive to feedback.[0] I have no relationship with the company except just being a satisfied customer. [0] https://github.com/codeapprove/feedback/issues https://github.com/codeapprove/feedback/issues
pokstad 3y ago
“Write Small Changes” - a software org that embraces this will go far.
hyggetrold 3y ago
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 to murder you.
- LeafItAlone 3y ago
  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.
  - hyggetrold 3y ago
    > 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 get additional review. In my experience, the killer combo for pairing is to combine it with test-first programming. The tests help create a shared construct for discussion and understanding. I freely admit I have a very strong pro-pairing and pro-test-first bias. But in my defense it's based on almost two decades of experience.
    - Xeamek 3y ago
      >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.
corethree 3y ago
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.
- singron 3y ago
  They had Google Code. I think it might have first been deprecated before Critique was popular (the prior code review tools at Google weren't quite as beloved). Also since then, buganizer was made partially public facing. It sounds silly, but they could relaunch a software forge suite and it would almost definitely be better than GitHub, and they could cross sell cloud and probably not even have a free tier.
slalomskiing 3y ago
That sounds impressive but does there exist a bad code review tool? Just thinking I’ve used probably 4-5 different ones and never had any issues
mkrishnan 3y ago
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 engineer showed a slight resistance. then this engineer removed himself from the review list (but this engineer is the only one who is not manager in that code owner list) and made the junior engineer to beg on his knee to provide approval example 2: - a engineer in my team in double click ad always puts tons of nit picks. he got easily promoted.the justification gives but the leadership that the engineer created extremes value. the leadership was from double-click acquired by google few years ago and they are extremely in-competent. example 3: - there is a tool in google which analyses code review comments and those reviewers are very argumentative are given badge that they are very throw in review and I have seen those stupid badges counted during promotions.
- noname120 3y ago
  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.
  - mkrishnan 3y ago
    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.
dpc_01234 3y ago
I don't really see anything miraculous here, other than "GitHub PR review UX sucks in comparison". Things like AI help and extra linters etc. are something that a each project has to figure out on their own, and e.g. I maintain projects that use Nix dev shells to optimize and streamline the dev experience by a lot, even before things hits the change review stage. Github's PR review flow just breaks down for anything non-trivial. People have been complaining about it for years and suggesting a lot of relatively low hanging fruit improvements. The rest is about the culture of CRs some of which could benefit from built-in support, but some doesn't need it.
danjl 3y ago
The biggest part of a smooth code review process are the human behaviors and best practices. There are a half dozen code review tools that work just fine. Both the submitter and reviewers have important roles which make the process work smoothly: small reviews, 80% of reviews should just be "LGTM", avoid nitpicking, code reviews have higher priority than your own development, etc.. Really, if you have good code development guidelines, and good developers, that solves most of the review issues. The high order bit is knowing that every piece of code will be reviewed. Any good programmer who knows their code will be reviewed avoids doing bad stuff.
cmrdporcupine 3y ago
Yes, Critique (and Gerritt) is a good tool. Yes, Googlers tend to behave well in code reviews and conduct them decently, but.. The reality is that this kind of survey is a) self-selecting for the people who adapted well to Google's process, and b) not really measuring for productivity, just satisfaction with tools and some aspects of the (assumed given) process. What I personally found is that Google is the kind of place where you can only truly be productive if you have the kind of brain that can fork off N work tasks at once and then fork/join on all of them. If you're the kind of person that does better when going down the whole mineshaft on one problem at a time, you'll be screwed. Because you'll be constantly spending the bulk of your time sitting waiting for review approvals, or getting your account added into some ACL somewhere, or getting sign-off on a PRD or design doc, launch / security approval, or some other form of coworker approval. The only way to make progress is to do a bunch of those kinds of things at once and juggle between them. After 10 years of battling it, I realized it's not for me, no matter the $$. (There are some other things that Google does exceptionally smartly, though. The monorepo and the way they handle third party deps [check them in and only allow 1 version for whole company]) I think is one. Both of these things seem crazy at first, but they are amazing at minimizing whole classes of complexity around versioning and dependencies that add mostly needless overhead to the job of engineers)
bvrmn 3y ago
Topic article basically shows screenshots with UI quite similar to Gerrit. It would be really great to get even a small glimpse of details from ex-googlers instead of "Critique is so much better than Gerrit". We have a kind of big repo for a single product with multiple teams working on it. For my company I've configured a few Gerrit QoL things: * Submit rules with different sets of required reviewers per directory. * Keep review labels on trivial rebases. * CI could be triggered manually by comment in a review. * Submit requires CI verified label. Gerrit has a nice query language to search for reviews. On top of that Gerrit provides patch-oriented workflow backing slick review process and keeps history clean.
- cmrdporcupine 3y ago
  I used both Gerrit and Critique. There are some similarities, and some configurations of Gerritt can be similar. But... Critique is not git based. And so a lot of the complexity around rebasing & commit management is just not there. Nor is the stuff about patch-oriented workflow, etc. It's a much simpler mental model (central monorepo built on a fork of Perforce, though they've added some DVCS stuff with 'fig' onto that since), and consistent across the whole giant monorepo. Some of the pleasantness comes from that. Some of the decisions and twiddly knobs in Gerrit probably come from Google teams wanting some of the Critique (or Mondrian before it) workflow, but on top of Git. The UI in Critique is somehow a little more consistent and fluid and attractive. Many of the keybindings in Gerrit come straight from Critique, and having keybindings is one of the things that makes both of them much nicer to work with than the crap in GitHub, etc. Gerrit is also extremely configurable, as you've pointed out. The query language is one thing they have in common with each other, and it's very nice. In my last year at Google I went back to working in Google3&Critique after working in Gerrit & git for some years, and I quite enjoyed it.
rapfaria 3y ago
> Unresolved comments represent action items for the change author to definitely address. These can be marked as “resolved” by the code author themselves when they reply to the comment. I've always told the team that whoever comments is the one that marks it as resolved (we use github). Otherwise they just gotta go and open every single comment again, and sometimes the code author doesn't even it address on big changes. This assumes the reviewer will comeback later to re-review it, of course.
yegle 3y ago
I don't see it mentioned, but there's a recent addition that greatly improved my turnaround time to review incoming CLs: A prompt will show up in the corner about the next pending CL that you can approve, when you give LGTM to the CL that you are reviewing. It's kind of like those "Customers who bought this also bought" prompts on e-commerce websites. Only here it slightly nudges you to have a "CL review streak" and clean up your review queue.
- dgellow 3y ago
  I never thought about this but it’s a fantastic idea. Would love to have something like this for GitHub.
  - SAI_Peregrinus 3y ago
    GitHub Notifications sort of does this, though it's not just for PRs where you've had review requested (it's gets noisy because it also includes things like PRs merging or being closed).
deleted 3y ago
[deleted]
makerofthings 3y ago
One thing I like about it is that reviewers can suggest changes and you can accept them inline. Makes it really easy to deal with nits.
- eftychis 3y ago
  You can do that in Github, but for some reason a lot of reviewers are not familiar or bother doing that. Fixing nits that way or giving a suggestion improves turnaround speed greatly and builds a relationship between reviewer and proposer and the final product/commit(s).
  - bbu 3y ago
    It’s not integrated well into the UI and thus a big hassle to use. Much easier to just checkout the branch and create a commit (or just write a comment…)
  - yohannparis 3y ago
    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.
    - froh 3y ago
      I feel stupid. How do I do this?
      - mplanchard 3y ago
        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
        3y ago
k_dumez 3y ago
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/ https://graphite.dev/
mstachowiak 3y ago
Shameless plug - I'm one of the creators of GitContext (https://gitcontext.com 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.
- aaravchen 3y ago
  Am I missing some of the features? The GUI does seem to be nicer, but functionally all I see thats added is support for non-blocking comments and explicit opt out of parts of the review. Maybe the visibility of what reviewers have already completed as well? The "pick up where you left off" is already available but requires you to manually indicate file-by-file when you've completed your review. The personal Todo list is also very present. What am I missing?
- mplanchard 3y ago
  Is this a desktop app? If so, is it cross-platform? I couldn’t find this info anywhere on your site.
  - 3y ago
asylteltine 3y ago
Why are googles UIs so ugly?
Sandworm5639 3y ago
> 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.
yeukhon 3y ago
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.
donaldagnes 3y ago
[dead]
Susan77 3y ago
[dead]

291 points by vinnyglennon 3y ago

manueljose46567 3y ago

[dead]

williamsherron 3y ago

[dead]

SuzetteClayton 3y ago

[dead]

seydor 3y ago

[flagged]

dontreact 3y ago

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.

shadowgovt 3y ago
It can be tricky to find a balance point here. At Google scale (in time-of-maintenance, not space... Code can last for years and will be worked on by multiple engineers disconnected from the original project), the hard problem of naming things becomes real. I find that it's useful to keep context. Even though one can never predict with certainty, sometimes you can be real confident that some code is prototype that will be thrown away in six months. And there's a big difference between the code that makes up an API layer and the code that implements a feature constrained to one module.
- arp242 3y ago
  At not-Google-scale code also often lasts for years; perhaps even more so since there's typically a lot fewer people maintaining it. I think the key thing is to ask yourself "is this really objectively better yes/no?" before commenting. Not that you can never comment if the answer to that is "no", but quite a lot of the time when the answer is "no" it doesn't really matter and it's just "I would have done it slightly different, but both are fine".
  - shadowgovt 3y ago
    Good point. Making renames a suggestion, not gating (unless it contradicts the name in some design doc somewhere that another team is relying on not to move) may be the balance point there.
    - arp242 3y ago
      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 pointed out that "dir" was a needless abbreviation so it became createDirectory(). Then yet someone else pointed out that it's actually recursive, and a discussion on the merits of createDirectoryRecursively() vs. createRecursiveDirectory() was inflicted on everyone. In-between there was a side-quest about directory vs. folder. Just fucking stick a fork in my eye already. Was anything of any objective value gained? I'm having a hard time seeing it. Well, it makes for a slightly amusing anecdote so there's that.
      - ryandrake 3y ago
        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, what a waste.
TheBlight 3y ago
Excessive subjective feedback can be soul-crushing.
- acscott 3y ago
  Agreed; after > 20 years of coding successfully, got hit by a storm of subjective feedback; it totally ruined any joy in development
  - TheBlight 3y ago
    It didn't used to be this way. A code review used to mostly function as a quick sanity check. Now it's basically code by committee.
  - wubrr 3y ago
    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.
    - runlevel1 3y ago
      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.
jeffbee 3y ago
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.
- dontreact 3y ago
  Im not saying it’s always nitpicking. I’m saying that sometimes it is.
- bheadmaster 3y ago
  "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 readable".
  - tikhonj 3y ago
    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?
    - bheadmaster 3y ago
      Of course, it's not 100% subjective - rarely anything in the world is 100% anything. But I think that most people consider their personal preferences to be better and more readable just because they're used to them, so I tend to take the opposite attitude as the starting point. There were more than a few situations where I've had a coworker tell me "just read the code, it's very readable", only to spend the next two weeks just trying to figure out how it works. Sure, once you figure out how it works and it "clicks", it's no longer (that) unreadable, but the fact that I have to spend so much time reading the codebase in the first place made me convinced that personal familiarity is a great part of what "readable" means.
seanmcdirmid 3y ago
you can mark your comment (in text) as a NIT and then unmark "Action required."
- dontreact 3y ago
  Yeah… but people sometimes dont do this
ayberk 3y ago
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++ readability was a much, much better experience. All the comments were about actually making the code better, eg, "use THIS_MACRO() instead of THAT_MACRO(), because go/...". I guess I think it's much more about the reviewer, and based on my anecdotal experience, the language :)
- tunesmith 3y ago
  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 opportunity for how to improve this area of the code", that's better then acting like you made a mistake in failing to change it.
  - xKingfisher 3y ago
    I try to avoid nits totally unrelated to the changes at hand, since on a subconscious level they may discourage people from even wanting to touch older/less loved files at all. The critical exception being avoiding issues due to path dependence. E.g while a change is "correct" is doing X poorly because of surrounding issue Y. So we should fix Y now instead of building atop it.
  - eichin 3y ago
    Something I find helps with this in particular is only allowing style comments with citations to an actual style guide item. (I talk about domain-specific style guides as "crystallized arguments" - we agreed on this and wrote it down, not because it's necessarily right (though it probably is) but that we really wanted to stop wasting time arguing about these particular things.)
egl2021 3y ago
Different languages had different pools of readability reviewers, so the expectations varied, but readability reviews were generally constructive and helpful. I was thrilled to have Ian Taylor review my go code. The non-readability reviewers were usually on your team, so there was social pressure in both directions. You wanted to learn and conform to the team's norms, and the reviewer couldn't be a total jerk about their comments. Everyone was generally on their best behavior.
Tyr42 3y ago
At least with the AI assist it's easy to one click accept the name change and be done with it.
chii 3y ago
> Sometimes it’s not worth arguing over small details like variable naming but naming is quite important. May be it's not that this is a nitpick, but that previous review tools prevent the fruitful discussion of names.

shadowgovt 3y ago

That ML-sidecar for recommending the actual change is a very correct place to put that technology: right in the line where someone is already reviewing the code, so they can confirm the ML suggestion looks correct before adding it. I can't overstate how valuable it is for reviewers to give the actual change they're recommending alongside the description, but it happens too infrequently if we make the reviewer write it every time. Actual code is the cleanest language to communicate ideas, but it's a time-sink and a mental drain to turn the English description into code snippets every time. Automating that is precisely the right place to save time for reviewers.

jdeaton 3y ago

Critique is nice but the ML suggested edits are a waste of attention and cant be disabled.

shadowgovt 3y ago
What's the current miss-rate on them? In principle, it seems brilliant, but I can see that value dropping off a cliff if more than half of them are worthless.
- michaelt 3y ago
  > What's the current miss-rate on them? Depends how good your code is :)
- hot_gril 3y ago
  Pretty low it seems. Occasionally I get obviously bad suggestions that are easy to ignore.
mattrb 3y ago
They can be disabled. Click the settings cog.
dpbriggs 3y ago
I disagree. They're pretty good at auto-fixing minor issues and suggesting best practices.

jeffbee 3y ago

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 practiced code review.

fishnchips 3y ago
To be fair to GitHub, there has been slow but constant progress over the last few years. I would love it to get at some point to where Critique was when I left Google (almost 10 years ago).
- charcircuit 3y ago
  I don't see any desire for them to actually improve it. It's been over a decade and the code you are actually reviewing is not even on the page of a PR. You have go to a separate web page to see it.
codeapprove 3y ago
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 https://codeapprove.com) to bring a lot of Google's best code review practices to GitHub. It doesn't give you everything Critique did, but I think it brings the same speed, clarity, and focus in a way that's still compatible with the rest of your GitHub workflow.
- stavros 3y ago
  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.

yx827ha 3y ago

If you want something similar, check out Gerrit: https://www.gerritcodereview.com/ https://www.gerritcodereview.com/ It's open source and used by Android and Chrome.

jasonlotito 3y ago
Yeah, it seems like Gerrit with lots of Google-specific stuff. Not surprised. Used Gerrit for 12 years, and loved it.
- nappy-doo 3y ago
  Gerrit is pretty crap compared to critique. It has a workflow that works for Android, but critique is really much better.
  - cmrdporcupine 3y ago
    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.
    - crotchfire 3y ago
      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.
  - k_dumez 3y ago

tallowen 3y ago

I’m always surprised to see the totality of support for this workflow. My biggest gripes were: - Owners - Readability review (or really the 18month queue to get Java/python readability) Although seemingly innocuous, this made maintaining internal libraries very challenging. There was no way to update every call site of your library in an efficient way. Tools like Rosie were added on top so that you could shard your PRs and shepherd many PRs through a Byzantine approval process. Compared to Facebook, I found it much harder to reach the same levels of productivity given Owners and readability review. I don’t think libraries like React could get developed at Google given how hard it would be to evolve the API surface.

jeffbee 3y ago
Google3 is a huge codebase of high quality that moves exceptionally quickly, so it just seems like your priors are getting in the way. Complaining that a large-scale change among tens of thousands of software developers requires a tool (Rosie) is sort of the same thing as all those people who complain that it's too hard to cope with having millions of machines in prod, i.e. the type of people who wash out of the company in a few months and then go on to a career of complaining about it online.
- marcinzm 3y ago
  The same applies to Facebook which is their comparison and not some random 10 person startup.
  - jeffbee 3y ago
    Except Facebook's codebase is an order of magnitude smaller.
    - tallowen 3y ago
      What makes you say that Facebook's codebase is an order of magnitude smaller?

throwawaaarrgh 3y ago

> 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 written it, get out of the way. If you're saying anything that isn't directly related to a necessary change to the code, get out of the way. When I review, if there's no bugs, and nothing that's going to affect performance, and it passes tests and works, I approve it. If somebody wants mentorship I'll add my thoughts in a comment along with my approval. I don't have this kind of authority, but I would also recommend just not requiring a PR for certain changes. Create a guideline so that changes which a person couldn't improve by reviewing or that have no impact on the working product are just merged. More merging, less useless blockage.

onlyrealcuzzo 3y ago
Code review at most companies is an ego thing, for reviewers to feel self-important. It's a way for senior people to pieces of crap to everyone underneath them because that's what they put up with for most of their career - and now they feel like it's their turn to be a piece of crap back and perpetuate the cycle. So many industries run on this exact same cycle. At Google, the company does a good job convincing you that you're in the top x% and lucky to be there. People generally don't really feel the need to crap on people in code review to make themselves feel self-important.
- sanderjd 3y ago
  I never know whether I've just been fortunate in my career or what, but I've just never experienced anything approaching the kind of unkindness that I frequently see people say is common. I dunno, maybe it's just me, but I'm skeptical. I've always worked with professional colleagues who are trying to figure out how to do their best, and have never once seen people behaving in this childish way.
  - 3y ago

raverbashing 3y ago

Wait... this looks like Gerrit with extra plugins The experience can be nice sure, but Gerrit is not something I'm too enthusiastic about (and yes, Github is better in some aspects, Gerrit is more configurable though and it seems it is smarting tracking "non official" merges)

compiler-guy 3y ago
Gerrit is a released version of Google's code review tool called Mondrian--the predecessor to Critique. Critique is basically Gerrit 2.0, although Gerrit has evolved somewhat independently.
- raverbashing 3y ago
  There was also Ritveld (ugh) before Gerrit And yes, I won't say much about Ritveld besides: ugh
barrkel 3y ago
Gerrit is a poor man's Critique and it's not nearly as slick to use in practice.

abtinf 3y ago

Are there any examples of successful organizations, maybe even with SOC2 compliance, that don't have code reviews? From my research, I think Netflix might not have them, but I'm not sure.

seadan83 3y ago
Instead of gating at review time, an org could gate at deployment time to achieve that SOC2 compliance. It appears that Netflix does do CR: "Netflix uses a feature branch workflow, where developers create new branches for each feature they're working on. These branches are then reviewed through a pull request process, where other developers can review the code and provide feedback" [1] [1] https://medium.com/@seyhunak/learning-best-practices-from-netflix-tech-stack-ci-cd-pipeline-a25a58f46711 https://medium.com/@seyhunak/learning-best-practices-from-ne...

willsmith72 3y ago

that ui sure is ugly. google really is an engineering company, and i love that, but it's still kind of jarring

qmarchi 3y ago
It's definitely looking better than even a year ago. There's a lot of internal UI reworks ongoing to bring things to consistency.

compiler-guy 3y ago

Worth noting that Google engineers had this level of satisfaction long before the mostly useless AI suggestions came on the scene. Those additions are comparatively recent.

teaearlgraycold 3y ago
My experience with the AI recommendations was mostly positive as an L4 doing Java
vinkelhake 3y ago
I find them generally very helpful. In a majority of cases, it's able to translate my review comments into the exact delta that I had in mind. Source: me, C++ readability reviewer - this means I review code from lots of different people, all over the codebase.
- andromeduck 3y ago
  It takes way too long though - it's almost always quicker to just do it in cider/subl if there's more than 1 of them.
summerlight 3y ago
I think the AI-powered suggestions are often useful, around 30~40% accuracy. The number might seem underwhelming. But when I work on code written in unfamiliar language/frameworks, this saves lots of my time because it gives some hints on how a possible change looks like and which keyword I need to look up for.

SpaceManNabs 3y ago

What is wrong with bazel?

tgsovlerkhgsel 3y ago
Bazel is a build system, not a code review system. Entirely different thing.
- SpaceManNabs 3y ago
  You should open the article. The very first content in the article is someone praising critique and also saying they wont use bazel.
  - tgsovlerkhgsel 3y ago
    Thanks - missed that because it was in a picture. I even went back and CTRL-F'd before I replied but of course that didn't work either...
  - aerzen 3y ago
    It's not easy to setup and has a steep learning curve. But it's great to use afterwards.

codeapprove 3y ago

Shameless but relevant plug: I built CodeApprove (https://codeapprove.com https://codeapprove.com) to bring the best parts of the Critique workflow to GitHub PRs. Obviously there are many things that don't translate (Google doesn't use git, and it has insane CI/CD integration) but CodeApprove gives you the same workflow. You always know which conversations are resolved, which PRs need your attention, etc. Feel free to reach out via email if you're curious.

mtlynch 3y ago
I'm a former Google developer and current CodeApprove customer, and I just want to endorse CodeApprove. They're the closest thing I've found to Critique outside of Google. The UI is really straightforward, and Sam (the founder) has been responsive to feedback.[0] I have no relationship with the company except just being a satisfied customer. [0] https://github.com/codeapprove/feedback/issues https://github.com/codeapprove/feedback/issues

pokstad 3y ago

“Write Small Changes” - a software org that embraces this will go far.

hyggetrold 3y ago

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 to murder you.

LeafItAlone 3y ago
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.
- hyggetrold 3y ago
  > 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 get additional review. In my experience, the killer combo for pairing is to combine it with test-first programming. The tests help create a shared construct for discussion and understanding. I freely admit I have a very strong pro-pairing and pro-test-first bias. But in my defense it's based on almost two decades of experience.
  - Xeamek 3y ago
    >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.

corethree 3y ago

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.

singron 3y ago
They had Google Code. I think it might have first been deprecated before Critique was popular (the prior code review tools at Google weren't quite as beloved). Also since then, buganizer was made partially public facing. It sounds silly, but they could relaunch a software forge suite and it would almost definitely be better than GitHub, and they could cross sell cloud and probably not even have a free tier.

slalomskiing 3y ago

That sounds impressive but does there exist a bad code review tool? Just thinking I’ve used probably 4-5 different ones and never had any issues

mkrishnan 3y ago

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 engineer showed a slight resistance. then this engineer removed himself from the review list (but this engineer is the only one who is not manager in that code owner list) and made the junior engineer to beg on his knee to provide approval example 2: - a engineer in my team in double click ad always puts tons of nit picks. he got easily promoted.the justification gives but the leadership that the engineer created extremes value. the leadership was from double-click acquired by google few years ago and they are extremely in-competent. example 3: - there is a tool in google which analyses code review comments and those reviewers are very argumentative are given badge that they are very throw in review and I have seen those stupid badges counted during promotions.

noname120 3y ago
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.
- mkrishnan 3y ago
  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.

dpc_01234 3y ago

I don't really see anything miraculous here, other than "GitHub PR review UX sucks in comparison". Things like AI help and extra linters etc. are something that a each project has to figure out on their own, and e.g. I maintain projects that use Nix dev shells to optimize and streamline the dev experience by a lot, even before things hits the change review stage. Github's PR review flow just breaks down for anything non-trivial. People have been complaining about it for years and suggesting a lot of relatively low hanging fruit improvements. The rest is about the culture of CRs some of which could benefit from built-in support, but some doesn't need it.

danjl 3y ago

The biggest part of a smooth code review process are the human behaviors and best practices. There are a half dozen code review tools that work just fine. Both the submitter and reviewers have important roles which make the process work smoothly: small reviews, 80% of reviews should just be "LGTM", avoid nitpicking, code reviews have higher priority than your own development, etc.. Really, if you have good code development guidelines, and good developers, that solves most of the review issues. The high order bit is knowing that every piece of code will be reviewed. Any good programmer who knows their code will be reviewed avoids doing bad stuff.

cmrdporcupine 3y ago

Yes, Critique (and Gerritt) is a good tool. Yes, Googlers tend to behave well in code reviews and conduct them decently, but.. The reality is that this kind of survey is a) self-selecting for the people who adapted well to Google's process, and b) not really measuring for productivity, just satisfaction with tools and some aspects of the (assumed given) process. What I personally found is that Google is the kind of place where you can only truly be productive if you have the kind of brain that can fork off N work tasks at once and then fork/join on all of them. If you're the kind of person that does better when going down the whole mineshaft on one problem at a time, you'll be screwed. Because you'll be constantly spending the bulk of your time sitting waiting for review approvals, or getting your account added into some ACL somewhere, or getting sign-off on a PRD or design doc, launch / security approval, or some other form of coworker approval. The only way to make progress is to do a bunch of those kinds of things at once and juggle between them. After 10 years of battling it, I realized it's not for me, no matter the $$. (There are some other things that Google does exceptionally smartly, though. The monorepo and the way they handle third party deps [check them in and only allow 1 version for whole company]) I think is one. Both of these things seem crazy at first, but they are amazing at minimizing whole classes of complexity around versioning and dependencies that add mostly needless overhead to the job of engineers)

bvrmn 3y ago

Topic article basically shows screenshots with UI quite similar to Gerrit. It would be really great to get even a small glimpse of details from ex-googlers instead of "Critique is so much better than Gerrit". We have a kind of big repo for a single product with multiple teams working on it. For my company I've configured a few Gerrit QoL things: * Submit rules with different sets of required reviewers per directory. * Keep review labels on trivial rebases. * CI could be triggered manually by comment in a review. * Submit requires CI verified label. Gerrit has a nice query language to search for reviews. On top of that Gerrit provides patch-oriented workflow backing slick review process and keeps history clean.

cmrdporcupine 3y ago
I used both Gerrit and Critique. There are some similarities, and some configurations of Gerritt can be similar. But... Critique is not git based. And so a lot of the complexity around rebasing & commit management is just not there. Nor is the stuff about patch-oriented workflow, etc. It's a much simpler mental model (central monorepo built on a fork of Perforce, though they've added some DVCS stuff with 'fig' onto that since), and consistent across the whole giant monorepo. Some of the pleasantness comes from that. Some of the decisions and twiddly knobs in Gerrit probably come from Google teams wanting some of the Critique (or Mondrian before it) workflow, but on top of Git. The UI in Critique is somehow a little more consistent and fluid and attractive. Many of the keybindings in Gerrit come straight from Critique, and having keybindings is one of the things that makes both of them much nicer to work with than the crap in GitHub, etc. Gerrit is also extremely configurable, as you've pointed out. The query language is one thing they have in common with each other, and it's very nice. In my last year at Google I went back to working in Google3&Critique after working in Gerrit & git for some years, and I quite enjoyed it.

rapfaria 3y ago

> Unresolved comments represent action items for the change author to definitely address. These can be marked as “resolved” by the code author themselves when they reply to the comment. I've always told the team that whoever comments is the one that marks it as resolved (we use github). Otherwise they just gotta go and open every single comment again, and sometimes the code author doesn't even it address on big changes. This assumes the reviewer will comeback later to re-review it, of course.

yegle 3y ago

I don't see it mentioned, but there's a recent addition that greatly improved my turnaround time to review incoming CLs: A prompt will show up in the corner about the next pending CL that you can approve, when you give LGTM to the CL that you are reviewing. It's kind of like those "Customers who bought this also bought" prompts on e-commerce websites. Only here it slightly nudges you to have a "CL review streak" and clean up your review queue.

dgellow 3y ago
I never thought about this but it’s a fantastic idea. Would love to have something like this for GitHub.
- SAI_Peregrinus 3y ago
  GitHub Notifications sort of does this, though it's not just for PRs where you've had review requested (it's gets noisy because it also includes things like PRs merging or being closed).

deleted 3y ago

[deleted]

makerofthings 3y ago

One thing I like about it is that reviewers can suggest changes and you can accept them inline. Makes it really easy to deal with nits.

eftychis 3y ago
You can do that in Github, but for some reason a lot of reviewers are not familiar or bother doing that. Fixing nits that way or giving a suggestion improves turnaround speed greatly and builds a relationship between reviewer and proposer and the final product/commit(s).
- bbu 3y ago
  It’s not integrated well into the UI and thus a big hassle to use. Much easier to just checkout the branch and create a commit (or just write a comment…)
- yohannparis 3y ago
  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.
  - froh 3y ago
    I feel stupid. How do I do this?
    - mplanchard 3y ago
      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
      - 3y ago

k_dumez 3y ago

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/ https://graphite.dev/

mstachowiak 3y ago

Shameless plug - I'm one of the creators of GitContext (https://gitcontext.com 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.

aaravchen 3y ago
Am I missing some of the features? The GUI does seem to be nicer, but functionally all I see thats added is support for non-blocking comments and explicit opt out of parts of the review. Maybe the visibility of what reviewers have already completed as well? The "pick up where you left off" is already available but requires you to manually indicate file-by-file when you've completed your review. The personal Todo list is also very present. What am I missing?
mplanchard 3y ago
Is this a desktop app? If so, is it cross-platform? I couldn’t find this info anywhere on your site.
- 3y ago

asylteltine 3y ago

Why are googles UIs so ugly?

Sandworm5639 3y ago

> 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.

yeukhon 3y ago

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.

donaldagnes 3y ago

[dead]

Susan77 3y ago

[dead]