All my developers like the fact that someone else looks at their code. It distributes the responsibility, facilitate knowledge transfer, puts emphasis on readability, ensures that plumbing and "boring" code is idiomatic and plainly result in less bugs.

Although I do agree with all of this. I sometimes feel like "distributes the responsibility" can actually cause things to slip through the cracks. The reviewer thinks "eh, I didn't write this, what I saw looked fine" and the author thinks "hey they approved it, so I'm not out on a limb here", causing both sides to look at the code less closely.

I'm not saying that always happens, but it does sometimes in my experience.

One team I used to work on had a really healthy way of dealing with this. Whenever a bug made it through to production, the team would have an informal post-mortem meeting where everyone who worked on the change in question would talk about the cause of the bug, and perhaps try to identify ways something like that could have been avoided.

There was no real interest in assigning blame to individuals. I think that was largely because we required two reviewers to approve every code review. So, when something did happen, it had to make it past at least three people. That automatically makes the mistake a team responsibility and not an individual one.

As a developer, I agree. With perhaps, like the sibling from ’city41, the responsibility. I wrote it, it’s my code. Maybe QA can have their share if responsibility, if any later issue is something QA could have found.

But it requires an environment where responsibility doesn’t equal blame.

This is also one additional reason why I don’t touch Github Copilot. Because I didn’t write that code.

Agreed absolutely.

This is also the sole reason I like sprints. They have storypointing, and that leads to shared understanding and better context switching throughout the sprint.

Yes! Too bad people rarely bother, nobody seems to have the time.

if code reviews come with responsibility sharing then the are going to slow things further, 100% agree with the rest.

How about dealing with "that guy" who will take you through 8 revisions until he basically wrote the PR?

I typically just identify and avoid, get reviews from other people instead.. would love to hear more advanced strats. Attempting to talk about it never works because they dig their heels in on "I'm just enforcing quality".

This is a challenging balance to strike. I used to work with someone who I considered "that guy", but there was no denying that the code ended up better for it, even if it would have been basically fine otherwise. Especially over the long term, those little things can add up.

On the reviewer side, I have sometimes been in the situation where I end up letting something slide because I didn't want to be "that guy". Then I end up reading the code later and needing to fix it.

I think the answer here unfortunately relates to power dynamics. I'm senior, and others believe I'm senior, so when I dismiss reviews like this, no one fights me. Sometimes, with folks like this, even without being senior, just standing up to them works.

As others have said, using linters, auto-formatters, etc often helps with this as a lot of the things these people like to nitpick are then standards enforced by code, and you can send them off to fight people about the standard ("but not in this PR").

Two things, first have a standard to compare to. Second, use code linters and formatters to automatically enforce it.

That obviates trivial back and forth, leaving the interesting issues.

You'd be surprised. Often that dude will have preferences that go way beyond linters and may even contradict widely accepted best practices.

I've worked with the dude who insisted on factoring out functions for unrelated 2-liners, creating extra coupling where we don't want it, the dude who insisted on mocking library code in tests when it would be easier and more thorough to just use it and make sure we're calling it right..

My favorite was the dude who, after 3 days of back and forth changes on a semi-urgent PR goes silent for a day, I ping him, he says "thinking of more comments :)" with the goddamned smiley.

Hah! The simple answer to that however, is "No." With a link to standards/best-practices if you want to be polite.

Unless dude is your boss, in which the answer is "Yes," and the paycheck arrives on Friday.

Also, sometimes other people are right... something to remind myself of. ;-)

Yes, or the guy who wants you to mock out database calls because it's not a "real unit test" otherwise. So you spend another day mocking out crap when it's both easier and less error-prone to use the real thing. I guess the time spent will pay for itself... in 10 years.

The point here is to be able to run the tests without a database, and in this case they're probably right. Saying it's for the sake of purity of the test type is wrong, but you should mock out database calls in unit tests because you want them to run fast and without dependencies.

Move those tests to integration...

We don’t run tests without a database. Never been anywhere that did. I greatly prefer integration tests, in general. They almost always have value.

90% of people are fine and code review has tons of benefits.. but the format is an enabler for 'that guy', as you're asking them for approval rather than running it by them as an FYI.

I've seen a few different motivated hard workers get to hard work on demanding as many changes as possible whenever they are on a CR.

There is also the flip side too where a block of code might just have a lot of problems and I’m trying to evaluate if it’s too hard in a public forum to flag all of them at once. If possible I try to flag a couple and to sit down and offer to do a one on one for more.

+1 to taking it offline when it's more than a couple minor style fixes.

I think it all comes down to judgement and there are definitely ways people can write code in their individual style without hurting maintainability.

That said, maintaining a codebase where there is a lot of variation in idioms, style and formatting can be a big headache.

It adds cognitive overhead and if you're changing files you constantly have to balance preserving the local style with other factors. E.g. if someone likes aligning their equal signs, and you add a new line where the LHS is one character longer, do you reformat the whole block?

I can't fathom to be honest to be back in a place where something like that is even a question.

We use spotless. We use prettier. Nothing passes the build if you don't adhere to the style rules. You may disagree with some stuff. Make a case! Change the rule definition, apply them and show the PR to a representative cut of the developer community. You might get it through. We've done a few like that in my time here. Or you might realize what that rule change really means in some cases and the PR is ultimately declined. Has also happened. Ruled looked good for the place that triggered the want. Sucked in most other places, so we decided not to do it.

If you have a guy that can't live with not getting his will 100% of the time my advice is to try and get rid of him as soon as you can. Or run yourself. Not healthy in the long run and frankly "I'm too old for that shit. Now get off my lawn"

I think it’s the “requiring” a review part that comes across as not trustworthy. A review isn’t required on my team, but we do it most of the time. There have been plenty of trivial changes I’ve made where getting a review would have been a waste of everyone’s time or just been “rubber stamped” anyway. I just my team and my team trusts me to make that judgement call.

The problem is the subset of devs who don't know what they don't know.

Dev 1: "I'm just going to reword this error message because I think it sounds awkward. Harmless, I'll ship it"

Dev 2: "Actually you've removed a keyword that our error messaging framework uses to highlight the error in neon red. We've seen in a/b testing that this bright color prevents error blindness 65% of the time, and saves users from having to fix mistakes that cost them on average 1 hour of confusion"

I thought you were going to say “now this string has to get retranslated and until then everyone will have a crappy experience” lol

Changing strings are about the most harmful thing to the bottom line where I work. :)

However, in your case, that should probably be automated so people can’t do that just like we helpfully provide similar already translated strings when you add new strings or change them. People shouldn’t be able to break production, review or no.

Haha, absolutely, it's a terrible example with terrible design that I pulled out of thin air, but it was just to illustrate the point of how even seemingly innocuous changes can have major consequences.

And the original author's point is that you can't possibly happen to foresee all of the consequences, and so the best thing to do is to just ship the change and deal with what breaks.

That's the whole point of continuous integration, and that's what the author is trying to push. That you actually move faster by letting people push and fix bugs versus push a change, wait some time for a code review (somewhere 1min...1day), go back and forth on some style things, maybe a the reviewer managed to win the game of "spot the bug," and then ship, and then fix any delinquent bugs.

Like, unless if that middle reviewer is doing something that really can't be done elsewhere, it just serves to slow things down.

> Actually you've removed a keyword that our error messaging framework uses

Hmm, so something is parsing a string looking for specific character sequences? I'd say the developer who changed that is doing the team a favor by surfacing a terrible practice.

Edit: NVM I see you acknowledged that in another comment.

The reason for requiring a review on a team with high trust is that you can trust your team to validate the assumption that a given change is “trivial”. My team has had several minor changes recently where mistakes were caught in the review process.

If your team thinks it’s a waste of time, they’ll treat it as one. But that doesn’t mean it is one.

In many companies changes must be reviewed for compliance reasons (e.g., SOC2 will generally require some sort of review procedure).

> a really unhealthy take

And yet the attitude that developers must have something looking over their shoulders lest they ship code that doesn't meet some arbitrary metric is ubiquitous in the industry. Just look at all the automatic code checking incorporated into CI/CD pipelines.

Of course some of the automated checking is both healthy and reduces toil. Security sanity checks, for example, But take a look at your own organizations CI/CD pipeline and ask "how many of these checks are there because someone, somewhere, didn't trust the developers to do the right thing?"

This lack of trust in development teams is especially rampant in outsourced work. It's an attempt to paper over the fact that shipping your development to a contractor on the other side of the world is a terrible idea.

Or you acknowledge that having a prettier and linter set up is a really awesome thing and you just make use of it.

At my place these things aren't there to tell you that you're a bad developer that doesn't care about well formatted and well written code.

They're tools that help you take out the drudgery of some of the formatting and style adherence. I am able to write code 80/20 clean and at the end I just run the prettier and linter to tell me about all the places I might have missed or 'not dealt with for now'. I even use out CI pipeline for this specifically because it runs the verifications faster than my local machine and I usually have other stuff to deal with anyway while I wait for that.

But then for various reasons the prettier and linter become a tool to measure the difference between expectations and reality. Metrics are collected, performance indicators are established, and goals set. Pretty soon the tool is no longer helping the developers, but is instead judging and evaluating them, feeding results into stack-ranking and PIP systems.

Sorry to say this but then you are doing it wrong (not you you, your company - and I get it some are like that but not all). Our prettier and linter auto fix where possible. I literally just run them before I commit and let them auto fix. That's it. Done.

Some linter stuff the cmd line can't auto fix but IntelliJ can for some reason. Some I need to do myself. So what.

Sometimes I forget and the build complains. So what, I run, git commit --amend and force push. Done.

Since I'm a manager too I actually sometimes don't even amend and commit a 'aaah crap forgot prettier again' and push that so people see that it happens to all of us.

Caveat: we also squash our branches and each PR is exactly one commit on master (rebase merge strategy). Meaning your branch is yours to do with until its merge time. Nobody minds what you do. YMMV.