A study of Google's code review tooling (Critique)
engineercodex.substack.com
engineercodex.substack.com
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.
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 :)
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).
This is not always the right question to ask. One can argue that both products are too cheap with respect to the value they offer, so the relative value between the two is irrelevant.
In other words, if you can afford $X and $2X without even thinking, and if you think even $10X would be a fair value for either, it doesn't matter if the $2X product offers only 20% more value. You would simply want to get the best, even if it's a diminishing return. I believe $9/month/developer can be classified in this category if you are actually doing code reviews.
Putting an svg filter over the video element is making the page render 1fps a second on Firefox MacOS.
I legitimately thought you uploaded a massive gif as your feature asset instead of a video.
If the effect is always on the video, you may want to just bake it into the video.
Otherwise, you might want to recreate the filter without using SVGs to do it.
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?
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.
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.
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.
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".
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.
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.
This kind of stuff was typical. At some point there was a lengthy discussion which prevented rolling out a rather important hotfix over:
while (true) {
if (someCond()) break;
if (otherCond()) break;
[..]
}
Oh, and then there was the great "evil incident". I had rewritten much of the frontend to this newfangled thing called "jQuery" that was all the rage. That was all fine and worked pretty well, but suddenly there was a bug hubhub about the jslint comment; the keyword to allow eval() was "evil" (one of those not-too-funny Crockford jokes), so at the top of /static/js/app.minified.js?v=12311 in prodiction there was something like:
/*! jslint: keyword1 evil keyword2 */
/*! MIT license blah blah */
minified_js()...
What I'm trying to say is that these people were rather obsessed with minor details to a point I've never seen before or since. Hell, there was a Company Blessed IDE™ that you had to use. Nothing else allowed. It was a complete piece of shit (IMHO) and after a few weeks I just used Vim. It worked. I got stuff done. No one was bothered by it. Still got comments I shouldn't be doing that...
While I didn't formulate "is this really objectively better yes/no?" clearly at that time, I'm sure it's been a pretty big influence.
No? You can clearly have loops that exit. Loops that can be infinite should only be so where you really want that to happen in those cases / are fine with it. "Infinitely until the user does X with no timeout" is a fair one.
The thing is, if I had written the conditions in the loop body it would have been fine with no comments. People were tripped over the "while (true)" and commented on that. That's okay, I can just change that, no problem. But the kept analysing and thinking about it to the subatomic detail. That as really the problem: small things tended to escalate to "a big thing" even when it was just a small thing. I don't know what it was – a kind tunnel vision? I don't know.
I once carefully suggested to paint a wall in a brighter colour during a HOA meeting. People objected. Fine, not a big deal and just a suggestion, I'm okay with the grey it was. They kept discussing it. I back-pedalled harder. People started to suggest we need to form a committee. I started to run. They asked if I wanted to join the committee "as I had brought up the issue". I overtook Usain Bolt. I heard they had several meetings. They choose the same grey it was before (or something so similar I couldn't spot the difference). Not my favourite colour or what I would have chosen, but it's fine.
for i in range(10):
Slightly less trivially, you can prove that
while true:
if x:
break
else:
break
But once the if conditions aren't exhaustive, it becomes difficult, perhaps impossible, to know that you won't end up in a weird unhandled state.
Preferring and even insisting upon code that is not only possible, but easy, to reason about is good. Or iow it should be incumbent on you to prove the loop halts, not on the reviewers to prove it doesn't.
> it should be incumbent on you to prove the loop halts
This is a nice one-liner platitude, but also completely unworkable.
This is the same construct, it's just using python syntax instead of c/c++ syntax. There aren't functional differences between the examples.
[Edit: To elaborate, your initial example translated to python is
while(true):
if x:
break
if y:
break
Writing code in a way that makes it more difficult for both humans and machines to divine your intent should be questioned in review! Perhaps it isn't possible to verify that (but that's also concerning), or perhaps you have some good justification for doing the unusual thing, but then that justification should also go in the code as a comment to give future folks context on why you have a strange and misleading pattern.]
> This is a nice one-liner platitude, but also completely unworkable.
Perhaps, in some cases (yes, we can't always follow NASA's coding standards), but ensuring that most of the time invariants are locally verifiable is possible, that's what typecheckers are, and it's not like Java or Rust are unworkable languages to write code in.
Just because we can't statically encode "this loop halts" as a type, doesn't mean that we shouldn't make that fact clear and verifiable by a human. And "it is incumbent on the person writing the code to justify why it needs an exception from <good practice>" is exactly what code review is for. It's not that hard at all. It's how I review code, and how I expect my code to be reviewed.
The "I" in that sentence suggests this should be considered subjective. And that's I think the cleave-point between gating and non-gating: "Other people have already agreed on this" vs. "In the moment, I, a single code-reviewer, think this name could be improved."
Other factors are if I'm the primary or one of the primary maintainers, the standing of the other person (also a maintainer or one-time contributor from another team), and things like that.
When it's not explicitly documented in the style guide and it doesn't violate some agreed-upon convention (which needs to be gleanable from within the file itself or it's not an agreed-upon convention... No "We know it was always done like that but now we're doing this," if you want to change it, you do the heavy lifting of putting together the consistency refactor to bloody well change it...), it's a suggestion. Non-blocking.
But taste does matter, so if an engineer is observed by their peers to be ignoring too many refactor suggestions with no explanation, it becomes an issue with the project lead and that engineer might get put on a shorter leash (in the form of "These are normally non-blocking... Except for Steve, he wouldn't know consistent and terse naming if it bit him on the FaceManagerFactoryService). It required enough human touch to understand what each others' strengths and weaknesses were, so more expensive in terms of mind-space, but I think it generated better results overall.
(And as the one whose code is reviewed: if you have a good reason not to change something and can express it, fine! You're a team member too and your opinion on how things should be also matters).
I don't care about style; as far as I'm concerned it's "do what thou wilt shall be the whole of the style guide" (well, within some obvious limits of reason). I don't even care about consistency all that much.
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.
But if the person is consistently leaving such comments under the guise of mentorship, 'raising the bar', or some other bullshit which boils down to them attempting to demonstrate their own seniority at the expense of other people's time and stress - then showing them how it feels is a great approach.
Bringing in other people and managers is not very effective I've found - it takes additional time, other people have their own stuff to focus on, and managers often don't have the technical expertise or confidence to push back against subjective comments which claim to be 'raising the bar' or whatever. It also doesn't look great when you have to bring other people in to help you address PR comments.
And, of course, you do this without ostensibly creating any conflict - if they complain simply respond along the lines of 'I totally love the care and attention to detail you bring when reviewing my PRs, I've learned from you and thought it would be appropriate to keep the same high standards and not lower the bar... etc'.
> As they go, if it isn't clear, I just ask them if something is a soft opinion or hard opinion.
Nah, if they don't explicitly state that's a soft opinion via 'nit', approving with comment or some other means, they are disrespecting the person who's PR they are reviewing. I shouldn't have to chase them down to see how strong their opinions are.
> 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.
What do you do if the comments are purely subjective and all backed up by internal/corporate dogmaspeak? 'Raising the bar' ... 'keeping the standards high' ... 'mentoring', etc. , or open ended comments asking to explain how stuff works, and whether 'this approach is the best'? There is no shortage of rhetorical bullshit that can be used to justify subjective PR comments.
> 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.
The above will generally not work in a company that emphasizes PR comment count as a good metric for promotions/performance, and has a lot of internal rhetorical dogma. You WILL get people who leave these types of comments because they view it as a way of promoting their career, these people often cannot be reasoned with logically because they aren't actually all that smart, and they view any pushback against their comments as an attack against them.
Other people's feedback against the bullshit comments definitely help, but can look bad if you keep reaching out to other people to help address PR comments - I made sure to go through other people's PRs, on my own initiative, and refute bullshit comments when I realized how some people were behaving.
And yeah it totally depends on the company/team.
How long did it take until you felt better again? If I can ask
Note that the important part is a good way to mark all those false positives that is simple and doesn't go to far. Just because I want a bad spelling in one place doesn't mean I want it everywhere. As a result I'm going to predict that your tool either results in too much boilerplate needed to suppress all the false positives, or your tool lets pass a lot of things that shouldn't. But that might be just that I don't have good ideas: if you create a good tool for this I'm willing to be proven wrong.
I bet GPT-4 understands that.
(Though I have no personal experience using it for code review.)
It doesn't directly relate to execution today, but it might later on when someone misunderstands the docs or just gives up on them and tries to hack it together blind.
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".
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.
Entire group of people has to think otherwise due to proliferation of that style.
Things like "lots of small functions vs. few large functions", or "exceptions vs. sum type return value", are such cases - they seem subjective, but they're less about preferences, and more about the kind of work a given reader is doing. Either choice is better than the other for some kind of work. We can't improve on this until we move past working directly on plaintext, single-source-of-truth codebases.
Plaintext is fine. Single source of truth is obviously needed. The problem is with insistence on only ever working directly on it, which leads to a futile attempt at inventing styles and languages that would express every cross-cutting concern and needs of every job in a clear and readable fashion. It's just not possible.
(And yes, I believe the bleeding edge of programming language development is effectively just spinning the wheels now - adding increasingly complex abstract math to programming languages isn't going to help square the circle.)
To put it this way, you should use Kotlin vals with get methods with getters rather than fun getXXX() functions, but this is controversial, so I always suggest it as a nit (Nit: maybe use val xxx get() = ... rather than fun getXXX() = ...).
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 :)
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.
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.
Even worse, if there are enough of these minor changes, some other reviewer may take issue with their "small, focused changes" preference, and ask you to split it.
This is why Google has the "It doesn't have to be perfect; it just needs to be better" standard: So reviewers can't impose undue costs on authors.
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.
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.
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.
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.
Graphite[0] is also similar in that space(code review platform built on GitHub), the CLI could use some work but combined with the web UI it scratches that same itch that Critique did for me
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 used to how to git to push changes to a Gerrit review so we ended up using Git extensions to manage that.
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)
Gerrit is awesome. I will never, ever go back to github.
They are two entirely separate codebases, built on two entirely different revision controls systems -- one open source, the other not -- with Gerrit inspired by Critique, not the other way around. Yes there are similarities be tween it and Critique. Because Googlers worked on both.
IIRC Gerrit is an open source re-implementation from scratch of Critique.
Source: me, C++ readability reviewer - this means I review code from lots of different people, all over the codebase.
After I left I created CodeApprove (https://codeapprove.com) to bring a lot of Google's best code review practices to GitHub. It doesn't give you everything Critique 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.
What would you pay for CodeApprove? Also if you email me I’m happy to set you up with a 6-month free trial with no credit card required. Maybe you’ll like it more than you think!
Yeah, that's probably true, it's just that I'm getting subscription fatigue, with every little tool like "Google Meet links for Slack!" wanting $5/mo/user. I commented something similar upthread, but maybe "free for the first five users, $9/mo/user after" would be much more aligned with your customers' incentives, and would allow you to get a foothold in companies when they're small, and be paid as they expand.
I don't know yet what I'd pay, as I haven't tried it and don't know what value it gives me (it might well be worth the $9!). I'd appreciate a trial, I could send it around the company and see if it makes reviews less painful.
Tiny details like thread filtering, thread replies not appearing as separate comments, "show changed lines" when a new commit modifies the subject of a comment, etc. Those add up to make gitlab much more usable.
Comparing between MR/PR versions, meta commands in MR templates, and so on.
Essentially the same thing exists in both GitHub and GitLab
Feel free to reach out via email if you're curious.
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.
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.
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.
I also agree that both have been pretty rare for me.
Another useful thing, in my opinion, is to use your "fresh eyes" to make suggestions on what could use to be clarified in code or documentation. "This wasn't immediately obvious to me, could you add a comment on why it works this way?". The author is often too close to the code to see that it isn't obvious, because it is to them, because they've already spent hours thinking about it. But this should almost always what be optional suggestions. (But, rarely, code is so unclear that it really shouldn't go in without being clarified or documented.)
But in general I do think it's important to consistently be thinking to oneself "my goal is to be useful to my team and organization. are my comments achieving that? would it be more useful to approve or to not approve this right now?". The temptation to be a fastidious editor can be strong - if most of us didn't have this impulse we'd probably be doing different jobs... - but I find this "usefulness" framing to help me a lot in resisting it.
And very strong disagree on not requiring reviews. It is much better to tirelessly foster a low-friction review culture.
+1 - It is easy to forget that it is possible to approve with comments. Not all review feedback should block a merge.
In my project we used to be allowed to approve with nits but recently they changed it that the code needs a re-review for almost any edits. The system has some criteria for which it will allow minor edits but it's strict enough that I've rarely seen it pass an edit. I assume there must have been some incident that triggered this.
Basically, an unaddressed nit doesn’t block merging. But any code changes will require fresh reviews.
If the change is very minor, it’s very quick for someone who has already approved to check the last commit diff and reapprove.
That said, if you can read the commits individually then final review for something where minor changes were added should be utterly trivial.
- Modify it and accept that this will trigger re-review
- Merge as-is... maybe they don't even agree with the nit
- Merge as-is and followup with a new MR under the same JIRA. This sometimes makes sense if it gets them into an important preprod environment, but they are really still working on the issue.
- Merge as-is and follow up with a JIRA on the backlog. Maybe they realized that the "nit" is really a bigger issue and should be addressed separately.
Regardless, there's value in not blocking the merge for non-functional issues.
But how will I prove my 'mentorship' abilities and seniority at FAANG without pretending my subjective nitpicks and lack of understanding are critical PR blockers? /s
Be extremely picky for PRs from new hires, so that they do things the way that your company does them. Make sure they put files in the right places, and adhere to the “flow” of the rest of the codebase. Also the sorts of style that aren’t picked up by a linter — like architecture choices — should be heavily scrutinized to make sure that they fit with your company’s paradigms.
After a while, they’re fully assimilated and you have a coherent style so that everyone is on the same page. Future code reviews go quickly because everyone makes similar architecture decisions and you’re never surprised. This also makes code easier to read if it all follows the same subset of patterns.
I’m not actually sure which way is better, but I think that both options have benefits.
Yes, juniors need guidance. That means you must read their code, and talk to them about it. They also need real-world feedback. They do really need you not managing all of the interaction they have with the real world. Now, what you can not manage is up for a complex risk analysis, but they really need you to get out of the way at some point.
E.g. before they start implementing a feature is better than after they finish it.
I think the word you are looking for is "hazing":
> haze (v): force (a new or potential recruit to the military or a university fraternity) to perform strenuous, humiliating, or dangerous tasks.
I had this at my current job. I almost quit because it was downright humiliating to get called an "idiot" in so many nice words. I don't do well with hazing... so I brought out quotes from text books, papers, and famous authors to point out how wrong they were.
If someone has legitimate concerns about the design decisions made, then they should voice them. However if they are refusing to adhere to guidelines, simply because they dislike the approach then that's being overly problematic.
A better solution is to actually sit with them while they build a feature, show them around the code, and answer questions. You know, treat them like a team member instead of making them prove their mettle.
I don’t see what’s so wrong about saying “you used inheritance for this relationship, but we have a pattern of keeping classes like this separate, since this system tends to change frequently. Please organize this like xyz module instead.”
Just a random example of something that a new person might do who is unfamiliar with xyz module and the complications there.
I agree that it’s good to mentor someone new, but honestly I think they still make most decisions themselves, and sometimes those decisions don’t match established patterns that they don’t know about. Ideally you notice sooner than a PR but I also think that people get busy and it’s ok to not have time to monitor everything a new person does. So sometimes it comes down to the code review to notice.
It’s not derogatory or hurtful, literally at all, it’s just pointing on that they did something in a way that goes against established patterns, and it’s teaching them what those patterns are.
But seriously, I'd ask why, and why again. I'm very much against cargo culting, and I will refuse to make changes in my PR if it is cargo culting. You're welcome to open a PR to my PR with changes if you feel strongly about it though.
Maybe this makes me hard to work with, but so far, I feel like it has led to better code and a higher velocity, everywhere I've worked.
Change is bad unless it's great. Being able to look around a codebase and know how things work because similar coding styles and patterns are consistently followed is a huge productivity boost (this is also what commenters complaining about the idea of readability elsewhere in this thread are missing). Something must be 10x better to compensate for diverging from those patterns.
What you write is not YOUR code. It is your TEAM'S code, and the good of the team is far more important than what you personally like.
I also disagree with it being "the team's code":
What you write is your code (and copyright law almost universally backs this up), what gets merged is a maintenance burden for all time. It very much matters what you like and don't like, and it very much matters that it is maintainable (whatever that means).
The team ... doesn't matter when it comes to code conventions ... they'll all be gone and moved on to other parts of the code/company/industry outside of five years. Most code lives long beyond today's team.
It's not like you're being told to never use for loops. Code review feedback such as the following is all objective, beneficial to the reader, and not worth pushing back against:
* This logic should live in [other component] * Our RPCs are named as GetFoo, not DetermineFoo * That function name does not make it obvious what this code does, please change it * This needs a test * This is untestable and needs to be refactored to support X
If anyone on my team ever described themselves as in "a war of attrition" with another teammate I'd fire them.
Same. Nor do I consider code review comments personal attacks, but anything that must be changed for "reasons," must be questioned. Not because it's personal, but because I legitimately want to know and I won't give up until I get a good answer.
> Code review feedback such as the following is all objective, beneficial to the reader, and not worth pushing back against
I'll agree that none of this is probably worth pushing back against, but it isn't objective. Very little is objective, in our industry.
> This logic should live in [other component]
Why? DDD will say one thing, MVC will say something different, though usually they are compatible or can be made to be compatible. You can also subscribe to hexagonal architectures and that might say something different...
There's nothing objective about where code should live, except on a hard drive or some other storage medium. I would argue that code probably shouldn't live on paper. I imagine most of us would agree with that.
> Our RPCs are named as GetFoo, not DetermineFoo
This is a nit. I'd honestly probably ignore it.
> That function name does not make it obvious what this code does, please change it
I'd probably ask for a suggestion because it probably looks obvious to me after staring at the code for so long. That being said, it might be a valid suggestion, especially if the code was refactored, but wasn't renamed.
> This needs a test
I hope nobody ever says this on my code reviews. However, I don't write tests for "obviously correct" code (code where the test implements the logic to test the logic): such as a function like:
function returnTrue(): true { return true; }
> This is untestable and needs to be refactored to support X
Do you know there is a such thing as legitimately untestable code (or at least, it shouldn't be tested in traditional unit tests)? Usually at the edges of two systems. For example, an API integration can't be tested fully, only the known contract from the other system. Then you start running into Postel's Law ... things get weird. Only if you have some kinds of guarantees with the other system (not usually), would I recommend traditional tests.
>This is a nit. I'd honestly probably ignore it.
Congratulations, until the end of time your team now has to remember multiple naming conventions, and when they want to do a code search for the RPC that retrieves foo, if they search for "GetFoo" because all the other RPCs are "GetBar", "GetBaz", and "GetTaco", they'll find no results and be confused.
"The team" is not a static thing, it is an evolving team-of-Theseus. Consistent naming and coding styles make it easier for the team, until the end of time, to read and search code. These "nits" are productivity multipliers.
As other reviewers have called out, I am a fan of "LGTM with comments", but any teammate who starts ignoring comments loses the privilege of receiving LGTM with comments from me and gets to wait until I actually review a version of the code that is acceptable to the team.
Do they not have PR's where you are from?
If someone is that annoyed by it, just open a PR. Just because I don't find it annoying, doesn't mean someone else won't. And vice-versa.
The code is mutable. It doesn't have to be perfect on the first iteration.
I will say this though: if you want me to do your nits, you also have to do mine. It's a two-way street when it comes to nits. If you don't do my nits, then I won't do yours. It's pretty simple. If you start withholding approvals because I stop doing your nits, then yay ... politics, I guess.
Once you land RPC DescribeFoo, I have to:
1) Define a second parallel RPC GetFoo
2) Wait for it to roll out to prod
3) Update all callers to call GetFoo, probably controlled with an experiment for safe rollover
4) Wait for all of those to roll out
5) Clean up all the callers
6) Clean up the DescribeFoo RPC
.....or you could just fix it in code review with half an hour of work.
Anyway. To me, DetermineFoo doesn't sound like a getter and seems like it might even possibly mutate some states. So, I suspect creating a GetFoo would be orthogonal to deprecating DetermineFoo. This means DetermineFoo might only Get a Foo by side-effect. That's why I would probably ignore it; simply because DetermineFoo might be a better name, in my opinion.
New behaviors occur all the time in software, but it doesn't mean we need to be rigid with naming things.
I wasn't sure if I was going to reply to this part or not. But you took a comment out of context here.
I'm incredibly touchy about code reviews. I used to have the popular opinion here where "the code belongs to the team!" and all that. And then I was stuck on a team with a very toxic guy who weaponized this stuff. His code was "perfect" and he'd approve your PR to turn around and rewrite it just to show how much better it could be (and gloat about it -- not realizing if he hadn't deleted half the tests, they'd be failing). He'd constantly point out all the things that were "wrong" and with the justification of "code quality" without actually being able to point out why it was better. After working with them for two years, the entire team's philosophy turned into the one I've shared here ... because it can't be weaponized. It can't be used to tyrannically rule the code. It can't be used to bully people around "for the sake of quality!"
His "code quality tyranny" did more damage to the code than any actually bad code could have done.
So yeah, I will go into a war of attrition over certain kinds of comments that are spoken authoritatively yet are entirely opinionated ... because I've seen what happens when it gets out of hand. I have no desire to go through that ever again. Ever. If that means someone like you fires me for it, it's better than going through that shit again. Sorry, not sorry.
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 computer science, or whatever paradigm we're using (DDD, hexagonal arch, actor-model, etc), then I'm perfectly happy to agree or spend a few minutes over a coffee discussing merits. But if the reason is 'quality' ... it's a "dual to the death."
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 PR was doing a type of snake_case to CamelCase conversion from some user inputs to some enums classes. It was done manually with String manipulation and playing with indexes etc. It also came with a bunch of unit tests to make sure the conversion was working correctly.
I put in a comment along the lines of "hey, this conversion seems like it can be done with this Apache Commons Text library; given that we are already using it in the same project in some other place, we can remove this manual code and call the library function; we can even remove the tests assuming the well-tested library is doing the right thing".
Was this comment warranted? If you think this is a reasonable comment, what computer science principle would you say it is based on?
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.
Except it hasn't at all. This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted.
The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it.
Successful projects don't just take any code that compiles and passes the tests, they are built in a thoughtful manner with high standards.
To be honest, I’m not sure how you got there from here…
> The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it.
I actually suggested deprecating the library. I am not going to presume how hard that is. It might be changing 2 lines of code or 1000. No idea. That’s why we have brains though, we are able to figure these kinds of things out. As far as ensuring it’s compatible with future plans… do you honestly think a “random” maintainer of a library has your best interest at heart? That’s rather unlikely unless the maintainer happens to work for the company in question. In this particular case, it looks like someone volunteered to be that maintainer (probably due to naivety, but that’s beside the point) and they actually do have your future interests at heart.
So to say that an employee doesn’t do work aligning with their company’s goals means you have bigger (political) problems than code review problems, hence why I said they should never have started working on it in the first place. For them to work on something like this for someone to review and professionally say “wtf” is a red flag, far and beyond code reviews; which is interestingly another reason why it is waaay too late to be having these conversations in a PR. It’s a smell that something in the team is fundamentally broken.
"I've done all this work, get put of my way and submit" is the attitude in both cases.
Congratulations, you wrote some code. It's not done until it meets team/project standards, and getting offended any time anyone has feedback makes you someone I'd never want to work with.
The default answer to code review feedback is always "sure" unless you have a strong argument otherwise. It takes way less time to implement feedback than to pretend you're Socrates and question everything.
That’s quite a stretch. Nobody should be “in your way” to begin with. In a company, you are on a team and the point of a team is to work together; not get in each other’s way. That’s a sign of a dysfunctional team, which is how you end up getting team members working on the wrong thing or implementing things wrongly in the first place. Code not up to “standards” is not a reason to reject code, it’s a sign that something else is wrong on the team and that is the thing to be addressed, not the code. You aren’t going to resolve it in a code review.
I don’t know how else to say that at this point.
> The default answer to code review feedback is always "sure" unless you have a strong argument otherwise.
I’ve definitely pointed out why a deleted line needs to be kept. I would not be surprised if that someone came back to tell me why it should be deleted. As a reviewer, I lack a lot of context even if I originally wrote the code. I’d never, ever, expect someone to blindly say “sure” on a code review. I don’t write perfect code and neither do my team members, so I don’t expect a perfect reviewer.
A team works together to make the code perfect, nobody is writing mystery code and doing weird stuff without discussing it with the team first. And sure, people experiment and we review it, but with an architectural lens. Then when we are happy with the architecture, we actually implement it, together.
Unless you agree on every detail before implementing anything, I don't see how this is practical. At least not in the companies I have worked in.
> The work has already been done. Try to capitalize on it, instead of dismissing it.
Not quite though. The developer may have written down some code but the work is not done. It needs to be shipped to production, monitored and supported by the oncall etc. If there is a way to not write some code and use a library, I will suggest that.
> 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.
If I see a PR with some key tests missing, I don't see why asking if we can add some more tests would be seen as calling them out on it. The PR is a place to record such things - may be it does not need such tests or may be it is intentionally not handled in the code etc. Why would such a discussion be seen as someone being an ass?
Why do I have to have that perfectly normal discussion in secret away from the rest of the team? We can ask questions and still be professional. Code reviews are not just for making sure code is good, but also for education - it is a good way for the other to know what is going on and also learn.
My read on this is that may be this is what happened to you - someone was being personal and attacked team members personally in the guise of a review and now you take a stance that either discuss everything before implementation in person or have separate meetings in private to suggest changes or ask questions.
I am thankful I did not have such a colleague/mentor when I started and hopefully I am not inflicting such an attitude to the new folks that are coming in now.
If your team is insulting you or saying that you're a bad engineer based on code review comments, that's a bad team. That doesn't mean that ensuring new team members learn the team's style and patterns is hazing.
If you don't suck it up, you're not "one of us" and you need to go. Or maybe they told you to do the wrong thing and see if you point it out. Who knows!?
>an arrangement of groups or forces in relation to one another
If you're being harassed in PRs the peer review system has failed. It's a place to make sure everyone is on the same page, not judge someone for how they code.
While I'm being pedantic here and get your point, I do think it's important that code is following proper styles.
I get that there's a bunch of nitpicky nonsense that goes into code review, but making sure it follows the same style so in 6 months when someone else has to dig through it you don't have to teach a totally different pattern/method or figure out what arcane bullshit they did is important.
And again, I'm sorta assuming this was implied but wanted to throw it out there since it's one of the most annoying things i've ever had to deal with.
We've internalized them where I work and I think they're really valuable. When it's abundantly clear whether something is a nitpick, suggestion, or a blocking issue, because it literally has a prefix to that effect, it takes a lot of the potential stress and confrontation out of the code review process, on both sides.
Note you also don't need to memorize the labels, but starting with one forces the reviewer to ask themselves the question "Should I really block the merge for this?"
I find it really crazy how long people take to do reviews. I have Github connected to Slack; if someone needs my review I get a message and can have it done within 5 minutes. (I think I write the most comments on code reviews of anyone in my organization, and I also have the lowest review latency. If I can do it, anyone can do it.)
And well, if you're primary motivation is Financial: okay, you're not being paid to move quickly. Or if you hope to move up the ladder quickly you gotta deal with the office politics. Which includes delays in communication. React to those lapses as you feel is appropriate to your goals and personality.
And even though it's "just a quick thing", if every time you're trying to get something done some "quick thing" gets in the way, it also gets incredibly frustrating. [1][2] Finding a good balance for this is hard.
What helps in my experience is building a reputation for small, easy to review changes. I know that I've left reviews from one coworker sitting for a long time because I either already took a look and knew that it was, or hadn't taken a look and dreaded it to be, a huge block of code that would require significant comments.
[1] https://www.reddit.com/r/ProgrammerHumor/comments/2rmir6/why... [2] https://www.reddit.com/r/ProgrammerHumor/comments/pafo1v/und...
Do you not do anything else? If I'm working on something else, I'm not getting to any new task within 5m (at least, not without a net loss of productivity to reload context)
Usually, the code review is the very last step before value becomes available to the business. There is absolutely no reason to delay a code review in this case, none, whatsoever.
Even if it isn't the last step, you're delaying the chance for a developer to continue working. More often than not, when a PR is submitted, there is a period where the submitter is twiddling their proverbial thumbs to prevent context switching from a reviewer.
So, delaying a review prevents value from being available and reduces overall developer productivity for the entire team.
Or, keep working on making an imaginary idea into a real one while someone else's already built idea sits there wasting money.
"I'm in a meeting with business owners". There, now either value is delayed or egos from the ones gaining value is bruised. Guess which one usually prevails?
We don't have to be this dramatic here.
Even if I was just working on another feature, that's questionable, because it only works if earlier-stage work might not ship at all; otherwise a delay at the beginning is the same as delay at the end, and overhead from context switching means you should prioritize whatever you're already doing.
But where it really gets absurd to claim that code review is always the highest possible priority is that I never said what I was working on at the time. Maybe I'm working on incident response - "Oh, sorry $BIGGEST_CUSTOMER, I know your prod instance is down and you're losing thousands of dollars per minute, but I just have to pop off for a bit because one of my peers needs me to code review a way to make forms load 0.2 seconds faster!". Maybe I'm working on a different feature, but it's one that will give the company more value (I have been in conversations to the tune of "we need this feature so we can close with this lucrative customer"). Maybe the other thing I'm doing is someone else's code review!
Now to be fair, I think there is real value in lowering the turnaround time on reviews. I could even see an argument for having someone who does code reviews to the exclusion of all else (which kinda sounds like the role you're playing in your company), provided that's known and factored in when handing them work. The only reason I think your position is unreasonable is that you've thrown out any notion of triage or nuance and pinned reviews to the very highest priority above all else.
That being said, I do make doing code reviews _the_most_important_thing_, except that I make it utterly transparent when I check for code to review (first thing in the morning and after lunch). I also make it clear that I'm only spending 30 minutes (max) on the review. If I can't review it in less than 30 minutes, I'm leaving a comment that the PR is too big and needs to be broken up and then not reviewing it (maybe someone else on the team is willing to review it).
That's my policy and the team agrees. Other team members have their policies as well (some won't review until they are also waiting for a review, some simply don't want to do reviews at all and only begrudgingly review and some are very similar to mine).
I think it is ultimately a conversation the team must have to have predictable releases and deployments. If you are doing nothing but code reviews for a couple of days before releasing ... you're going to have some fun integration issues and need quite a bit of manual testing.
Actual priority items are always owned by the whole team, not an individual.
The specific response auditors expect is “no, because another human reviews all change requests before they’re approved”.
As with most quality issues, the key is to try and surface quality problems as early as possible. Finding a bug in prod is worse than finding it in PR is worse than finding it with tests is worse than finding it at your desk is worse than finding it in the design. Engineering teams should choose an operating point in their processes that balances quality and velocity to match the business needs of their project. Most often I’ve seen slow PRs caused by stuffing too much of the responsibility for software quality into that single step.
In my team we prefix nitpicky comments explicitly with “nit:” and it’s up to the author to decide what to do with it. We minimize trivial nits by enforcing a style checker. We also discuss in retro if the PRs are too big to review effectively or if PRs are turning up comments on solution architecture, because at that point it’s too late—the author likely should’ve separately discussed the architecture before it got to a PR.
This is different from what the OP is talking about.
I've worked at places where staff engineers seem to have been rated on number of comments left on PRs... they were typically somewhere between nit picky and useless, with the occasional person directly contradicting feedback they'd given in a previous PR.
Oh, and you had to address every one of them before getting approval.
> Oh, and you had to address every one of them before getting approval.
This just sounds horrible.
It sounds idiotic.
It is unrelated to the question whether short-term (one PR) only reviews are preferable over reviews that recognize the reality that it is not the last PR ever and therefore more long-term view may be useful.
I've seen team-quarters wasted due to bad large-scale decisions. Sometimes the truth doesn't come out until the the real thing is in production, and all these extra burdens make it harder to change course. I think people are starting to get this because the amount of code nitpicking or caring about small things in general has gone way down.
Show me a project which this doesn't happen to on a long timeline and I'll show you a project with no users and perfect programmers.
Entropy is unavoidable. It will eventually degrade despite your best efforts and in the meantime you aren't benefiting from a working feature. We should try to delay software entropy as much as practical, but not at the expense of shipping.
Basically there is a balance to strike. I don't have a formula for it, but I do know that merging often ends up being more valuable than merging later just to ensure clean code. Working features are more valuable than a mess. A mess sucks, so we should try to avoid it, but the world runs on messes, not clean code.
This is always the approach I take. Especially for junior engineers. Stay out of the way, let them merge code with as little friction and discouragement as possible. Anything that I would have implemented or approached differently I try to elaborate on. Even go so far as to write some (most or all) of the code in a comment or in a remote branch to share. I will still ship the code but let the submitter digest my thoughts.
It's the time to get feedback and improve.
The problem is: if people are stuck and don't learn the code review phase will always stay as a long thing.
The whole team is responsible for security, maintainability, etc.
I don't care about some code monkey able to write some code or fixing something. I care about a team, a product.
Your attitude would not work in my team, which is fine
I very rarely see bad code in PR's unless it is from a Junior programmer, and even then, it's usually because they don't know better. Even then, I choose to trust them to do the right thing. In most environments, you can even review the code AFTER it has been merged. So, if they don't address the issue, I can still review the code and let them know that I expect to see a follow-up PR addressing it.
No. That’s literally the point of a review.
For starters, it results in a waste of time for literally everyone involved:
1. The person writing has to rewrite it (probably).
2. The reviewer could have sat down with the person before a single line of code was written.
3. Anyone else reviewing just wasted their time because it will be rewritten.
If you see code badly designed, walk over to, or call the developer and say "hey, can we discuss the design of the code."
The saying "measure twice, cut once" doesn't just apply to wood. Design before you ever write any code.
Me: I think we are getting events from SQS out of order and that’s why we are seeing some weird synchronization issues. What do you think about sending events to the regular queue and a FIFO queue at the same time and comparing them?
Team: How would that work?
Me: Since we are using a single threaded consumer here, I think we can simply override the transport class and instead of pulling from one queue per transport instance, we set up a feature flag allowing us to pull from two, compare the events and if they are different, alerting us in the logs.
Team: Sounds good!
You don’t need a full design spec, just agreement from the team on the approach. There won’t be any surprises in the review.
There can always be surprises. Sometimes when you see something implemented you can get a « click » as to why another solution would be better. It happens to me regularly both on the receiving and giving end of this and it’s rarely a big deal. Most of the time it happened to me, I could reuse the functional tests I had written and parts of the code anyway.
Reworking on something so that it’s better is never a waste of time, and it’s always better to do it before it reaches Prod.
We all see these suggestions. That’s ok, and wanted. But to say “ah, no I think it should be the other way.” [reject]
That’s what I was talking about here.
2. They could but they usually don't. It's fine to sit down and make sure they understand some esoteric structure within a system I owj I'm not going to sit down with someone else to make sure they use for each loops unless it's a very new junior (i wouldn't block that in most code reviews anyway unless the code was legitimately painful to read, but every company is different).
3. IME there may be multiple reviewers but one primary reviewer. Half the time the non-primary ones either focus on "is this code going to break your work flow?" or are simply there for bookkeeping and isn't the approval you're looking for (i.e. A lead on a review that already talked about the plans but doesn't have the knowledge to review that specific module). Anymore than 1 primary reviewer probably needed a smaller PR if possible.
I’ve seen and used everything from MVC to DDD to TDD to MVVC to whatever React is to reactive to event-oriented to actors to whatever you can imagine. There simply isn’t a “standard” way to do anything in this industry. If you hired me, you hired me for these different perspectives on problem solving, and that goes for all of us.
A code review simply isn’t the place to enforce your unique perspectives on someone else.
Sometimes, there is simply a better way to do things. These might even be new ideas in the code base. If it looks like it is, hopefully the author will bring it up with the team beforehand. If the reviewer changes their mind in the code review… well, that’s not the authors problem. The reviewer should rewrite it and the author review it.
Actually, that’s exactly what I suggest if someone ever tries that with me (usually when I’m the new guy). Please submit your own PR showing me how it’s done. I’ll move on to another ticket.
Usually, about half way through their implementation, they’ll see why I did it the way I did it and approve my PR. It only takes one time before they start asking “why” instead of assuming they know everything; every line of code exists for a reason, after all.
In one case, one specific dude (bless his heart) kept blocking PRs after we discussed everything before hand. I had to get HR involved, and other engineers. Dude just didn’t like being wrong and would throw tantrums when he was. He eventually got fired after people realized they could do what I was doing to stop the nits he said was a blocker. Nobody cared if a variable should be renamed from “SameFactory” to “EqualFactory”.
Ah, but code quality! It’s so subjective. High code quality, to me, is easy to maintain code. Is the intention clear, do the comments reflect reality, is it easy to read, but more importantly, easy to change? Will changing a line in the module break 15 other sibling modules? God I hope not. Dependencies should be obvious. And, no, I’m not writing an interface if there is exactly one implementation. That’s ridiculous.
For some people, they want layers. More layers of abstraction than a layered cake on a wedding day. To them, that’s good code quality! Some people think high code quality is beautiful code. The kind you frame and put over your fire place to admire with a glass of wine.
There’s no objectively “high quality” code in existence.
Yes, it matters what the code looks like. Knew a guy, named everything in his code a letter of the alphabet. a,b,c and when he got to z started za, zb etc.
That was 'bad code'. Or, it was 'Low Quality Code'. Nobody wants junk like that. Even if it passes tests, is robust etc.
I can think of a few tools that do that to your PHP or JS code, on purpose. They even cost quite a bit of money (obfuscation). Sounds like you might have gotten a good deal there ... /s
standard is relative, and while I'm talking generically as someone on the internet with no context of your experience, a code reviewer shouldn't. Someone literally saying "the code is not up to standards" needs to clarify.
For my generic take, take "standard" as "whatever that team has laid out in its code architecture". Sometimes there may be times to question the standards if it compromises other important factors, but personally I don't think the code review is the time to fight over whitespace and bracket placement (those should be solved via a linter anyway). Make the correction and submit, those later discussions can happen offline.
>A code review simply isn’t the place to enforce your unique perspectives on someone else.
Likewise, the goal is to align everybody, not to argue over semantics or enforce your own POV. That's why code style sheets are a thing; it's a mediator that can serve as its own battlground should team/company style need to be changed. Again, every place is different, but
1. style should be automated as much as possible. If there's some stupid whitespace rule, it should be a one button click on my IDE from some provided linter to resolve (even if I will proceed to re-lint it to my style afterwards on my local machine).
2. style sheets can have suggestions as well as laws. Be reasonable on what is what
>For some people, they want layers.
sure, lawful evils exist, both maliciously and inadvertently. Sounds like you ran into both kinds. Office politics are inescapable even at the best companies.
That sounds like another discussion to adjust to style sheet, not throw it out upright. If you see a bunch of examples of a law being broken, adjust it to a suggestion unless is a strong argument made otherwise. (most) companies aren't a congress where amending such things takes months of proposal, and we can very much adjust the document to the people if there's no resistance.
Now, do you actually care enough to spearhead that change? I imagine many don't, and there in lies the problem. That's why I'm not on management track; I don't have the care nor attention to worry about documenting such things unless someone throws it at me. I'll leave that to those who do care.
This is a bigger waste of time. If every change requires asking someone in person, that's an infinite source of distraction and kills productivity for that person.
MOST code reviews don't end up in comments asking for big changes, and when they do the one-time cost on the author is much better to pay than the constant tax on your subject matter experts of what you're proposing.
* Most CLs don't take multiple days to write
* Reasonably-sized CLs take 5-10 minutes to review - if I have things to say. Much less if the code looks good and I don't have any requests.
* I cannot think of any CL in the last 5 years where I made a comment that caused someone to "rewrite everything from scratch". Most feedback can be done in a few clicks in an IDE which supports refactor or a few minutes of copy/paste.
Besides, that, I mostly agree with your stance on code reviews. The way that most programmers do code review is so very "waterfall" and they don't even realize it.
It depends on the language. For example, an optimizing compiler can optimize the shit out of a for-each loop, but can only go so far in a for-loop. For example, in a for-loop, you can "skip around" the indices, and even go backward after going forward, while in a for-each loop, it will always go one-by-one, and the compiler can rely on that fact. Even the CPU can probably optimize it better since the data access will be more predictable.
If a unit of code fixes or improves the user experience without slowing it down and is memory efficient, then it becomes incredibly hard to objectively define said code as being bad. Perhaps it ruins the developer experience, but then again, many programmers object to things they simply don't like on a philosophical or theological basis. Programmers, depending on the language community, may consider anything that isn't "object oriented" to be poor quality, but good luck finding objective proof that object orientation is always better.
Not doing it every time, will come back to haunt me later.
Most of the times, the reply on my comment for a more obvious variable name is ( not: ) is: yeah, I thought about a better name but couldn't find any or something is found in between.
Ofc, sometimes the change is so ugly I'll feel bad because they should rewrite it. I'm still struggling with it.
Some people just can't code too. These will come back to haunt us.
And will have bugs too...
Sure, technically, it can be thrown out the door, and often is, in just this form, but it's a shit-tastic mess without QA time and the users are getting real tired of half-assed garbage that cost twice what the prior package cost.
What I try to do with my code reviews is prefix my comments with "Non-blocking suggestion:" or "Minor question:" so that the other person is more likely to know that a particular piece of feedback doesn't come with the expectation of needing to be addressed.
Otherwise, if the code works, has tests, and isn't objectively incorrect, I try to give an approval. If you don't like something about it, there should be room in the process to make improvements. The idea that "mistakes won't actually get fixed later" is a cop out. If your team can't fix mistakes, then you should turn in your "engineer" titles.
Also, when programmers do things that others on their team disagree with, it almost always comes down to a breakdown in communication. Junior developers are a bit of a different story, but senior developers not knowing how to write code that the rest of the team approves of means that people aren't communicating, and it probably means that your system is so complex that it's unreasonably difficult to determine the proper approach in the first place.
These are all things I think are valid to point out in a code review. I don't think I would like to work in a company that rejects the notion.
- 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.
[0] -https://www.wired.com/story/facebook-zoncolan-static-analysi...
[1] - https://www.wired.com/2015/09/google-2-billion-lines-codeand...
That's more of a feature and not a bug to me. Users of a library would appreciate it if the API surface doesn't change dramatically over time.
Don't know what the alternative would be.
Furthermore, there were tons of changes that could happen across the board at facebook that didn't happen at google due to owners files. Lots of people were empowered to make broad changes to the entire codebase which in aggregate made the codebase better. Coming from Facebook, all the systems around owners and readability represented a huge amount of time to be able to make changes that should have been easy. It also left the codebase with many warts that would have been relatively quick to fix locally but a pain to land.
Facebook went in the other direction - there were better and better tools for making broad updates
Now, finding somebody with authority to approve is a constraint of code review. But that would be true of any code review tool. So it's not really fair to complain about.
With the crazy interview process, the copious static analysis and formatting tools and style guides + style enforcement tools, and with OWNERS and team members reviewing things... what is even the point? What are they actually checking for?
Back when I started there, I got my C++ readability quite easily with just a month wait or so. Then I briefly worked with a bit of Go, and encountered my first taste of the incremental process, and it.. sucked. When they rolled out incremental readability, I never bothered with any other languages.
Luckily, later, I ended up working in the Chromium tree which required no such readability status.
- OWNERS is absolutely a necessary and important thing, and yes it sucked when it made finding an approver hard, but the point of OWNERS was to optimize for _local_ ownership. (For non-Google folks: think CODEOWNERS files, but hierarchical/recursive, so approvers in /OWNERS, a/OWNERS, a/b/OWNERS, and a/b/c/OWNERS can approve changes anywhere in a/b/c/...)
- I joined in 2017 and it never took 18 months to get readability. If it took you 18 months, it was because you spent 2 weeks writing code in one of those languages and then didn't keep it up. I worked through the stats on this too- by the time I left in 2021, there was virtually no delay to enter the readability process for Java and it was maybe a few weeks for Python.
> There was no way to update every call site of your library in an efficient way.
Does FB have something better here?
G had sufficiently many tools for this, IMO: csearch+grep+sed for the easy changes, Refaster/clang-tidy for more complex stuff (admittedly C++ AST matchers are black magic that few people on even the C++ teams understood). Although I do wish I had known about Comby before I'd left.
Rosie - the tool for executing large-scale changes, i.e. if you made a change to 1K+ files (often 10K+ or 100K+), you needed a way to break the change up into multiple PRs - was also an absolutely critical part of this. I never found the approval process Byzantine - undocumented, perhaps, but remarkably streamlined considering that LSCs meant making simultaneous changes to code in Ads, Android, Cloud, Geo, Search, Technical Infrastructure, YouTube, or however Google breaks down the engineering these days.
It makes it much easier to rapidly iterate and improve shared libraries.
> There was no way to update every call site of your library in an efficient way.
There are global approvers. It's not super easy to get their approval (you need to go through the large scale change process), but getting each owner's approval will be exempted if you can get the one.
Ownership is the easiest issue to solve here. Beyond a few dozen clients, you're going to run into the inability to run tests fast enough, and beyond a few hundred clients, you won't be able to sync fast enough, even if you skip all presubmits (there will, in expectation, always be a merge conflict in some file). 3-stage migrations/rosie are necessary at that point anyway. Owners is only the problem at like, 5-10 clients, and when I made changes at that scale, messaging owners was usually fast and effective.
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.
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.
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)
Depends how good your code is :)
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.
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.
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.
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.
I understand what you mean by this, but in my mind it's a poor solution because it doesn't fix the root cause. The root cause of stacked PRs (in my experience) is that people are waiting so long for review that they branch off the initial change.
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.
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.
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.
It's like the company is run by both geniuses and clowns.
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.
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.
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)
And yes, I won't say much about Ritveld besides: ugh
From my research, I think Netflix might not have them, but I'm not sure.
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-ne...
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
Just thinking I’ve used probably 4-5 different ones and never had any issues
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.