Actually, someone is paying the team to solve business problems and create revenue.

Preferably in the most efficient way possible (which is of course subjective). I believe that after 3-6 months in a particular subsystem, the ROI of code reviews increasingly decreases over time.

But shouldn't the time required to review also drop significantly when the reviewer is familiar with the module?

The people who pay my team mostly want bugs fixed. Bugs going in testing are far more expensive to fix than those found in code review.

>Bugs going in testing are far more expensive to fix than those found in code review.

How so? Particularly if you don't do your code review right after development. That holds true for production vs. QA, but I'm not sure it does for QA vs. code review, unless your QA deployment and ticket bounce back process is overly cumbersome and you don't have time planned for it.

We always do code review before QA. But our QA process is also multilayered, first layer is QA engineer, next layer is first level business acceptance testing, then another higher level business acceptance testing. Gets pretty expensive once the SR marketing person rejects.

No i didn’t design this process.

"They should be the best tool you have for teaching and communicating best practices. "

For christ sake, no. If you want communicate new best practice, do so. And if team agrees, we will follow. Don't ambush me with new expectations during code review and don't keep my task hostage in order to push for your new best practice.

Same with teaching. Teach proactively instead of pretending juniors can solve it and then rewrite all their decisions in code rewiew.

If you have expectations, tell them in advance. It is just unfair to use code review for that.

That reminds me of a SQL code reviewer at my last company. He'd have a standard that we would follow and finally get stuff passed most of the time. After a few weeks, he would read a new article and completely change his passing criteria without telling anyone, so all of our previously passing submissions would then fail. This happened several times over the course of a year. It was very frustrating and rarely helped the overall quality of the SQL.

> If you want communicate new best practice, do so.

I think the quote your replying to means existing best practices. IMO, things like "we tend to follow this naming convention" and "here's a useful language feature you may not know about" seem completely fine to say at code review time, and it's not like you can teach someone absolutely everything before they get started.

Having a reviewer hold a review hostage is a big cultural red flag, IMO. If there's a real significant disagreement, you shouldn't try to solve it via code review comments; you should move to in-person communication if possible and maybe pull other people in to take a look. I also think that in most situations, reviewers should bias toward suggestions and ideas rather than demands ("strong opinions, weakly held"), and authors should feel comfortable pushing back or proposing to merge the commit as-is and leave some change for a follow-up review.

Saying code reviews are "the best tool" for teaching and communicating best practices is maybe a stretch; I find things like pair programming to be much better for stuff like that. But I still think code reviews can be a great mechanism for teaching code-level details like code style.

I have zero expectations on formatting preferences. But i have high expectations on writing correct, maintainable code. If i see code that can fail in ways you haven’t anticipated, or is a duplication of other code in the project i’m going to point that out. If you implement something in a way i wouldn’t, or can’t understand, it may be an opportunity for you to teach me something.

And if I tell you to re-implement working code my way simply because of some personal preference, i would expect you to tell me to fuck off. Cause that’s what i’d say if you did it to me.

To give you an idea of how our code reviews are perceived, we have one junior dev, at last retrospective she spoke up about how much she valued and appreciated them.

Obviously some places don’t get it, but i think we do.

I don't mind formatting requirements - when they are known in advance and same for everybody.

But last time there were two "seniors", each with different conflicting preferences and each insisting that your code is "bad" and "mess". (And inexperienced project manager buying their bs because they were confident). And it changed based on what blogs they read last week. Juniors were more fine with that then me, because they tended to belive seniors know what they are doing. Plus juniors worked only on one module and I did on two, so I could see how what one called crap on the meeting is demanded by the other.

And the rules were different for different people.

I think it is the ability of coder to tell "fuck off" on changes he disagree with that makes all the difference. I was comiting code I disagreed with and had to come Sunday to make changes I considered useless.

I used to be for code reviews, but now I think that it requires people with strong self awareness, knowing difference between preference/feeling and objectively betters, and many programers out there don't have that. The seniors I had trouble with were on spectrum, one a little bit other a lot. They could not tell the difference and it cause a lot of problems beyond code reviews.