[spacemadness]

Every time some code reviewer comes into my PR and says something along the lines of "you know you can just write it this way" where "this way" means obfuscating the code because "clever" and "shorter," I die a little on the inside. This is from experienced devs who should know better. At one point I wrote a comment write above a section I knew would be targeted by this kind of thinking explaining it must be written this way for clarity. Sometimes that works.

[kstrauser]

I only recommend that if the more explicit code is less idiomatic. For example, if someone appends to a list in a Python loop where a list comprehension would express the same thing plainly, I’ll suggest it. That’s it.

Otherwise, please optimize for writing stuff I can understand at 2AM when things have broken.

[Shocka1]

In one of my first positions out of undergrad we had a few devs on the team that got overly caught up in stuff like this. They were no doubt smart people, but their egos got in the way of things way too often. I'm not kidding - we'd get caught up on an if statement curly bracket for a ticket and there would be an argument for 30 minutes to an hour over whether the curly bracket should be there or not. These arguments would go into full blown dissertations, evolving into tangents of BSD coding style or Doom code. Keep in mind this was on a very well known automotive software platform with 20k bugs and counting open.

There is definitely a time and a place. Those were extremely frustrating times and a great learning experience for a young dev.

[ungreased0675]

I’m not a dev, but I manage them. On one team they were spending many hours on code golf and nothing was being built. I pushed the devs to passing testing=PR accepted.

In your opinion, what problems might come from removing opinionated code reviews? Why do some reviewers gravitate toward “Here’s how I would have written it?”

[karlmeissner]

Often, different approaches can be used to solve a given requirement. So debate is needed.

But it could be that your team was just divided on approach and style.

They were struggling because they were trying to work out their differences through PR comments. That will be frustrating for everyone. Somebody went and got the PR working "the other way," and now the reviewer is trying to get the author to change the PR to "their way". If it goes on long enough, your devs will head for "the highway"...

If you just mandate "test pass = pr accepted," it will unblock your team short term, but in the long term, the large system will gravitate into many tiny, fiercely defended fiefs, each with different styles. Maintenance will be slow. Debugging will be complex. Wide-reaching refactors will be prone to blockage.

Fix the coding by having the team spend time "not coding." Establish a proposal and design process where an author needs to get team buy-in before they can implement. Establish an accepted standard on coding style. Do wide reviews on critical features and highlight issues where flow needs to go from one end of the system to the other and there are weird boundaries. Do the RCA (root cause analysis) and ask the "five whys". Look for patterns of issues and address them soon rather than pushing them to the back of the backlog. Prefer many small incremental changes over few large changes.

[consteval]

I think the biggest problem that can come of it is lack of standards.

Each dev has their own way of writing things, their own little language. To them it is perfect. And it all works, passes tests. But, if you let them do this then your code becomes sloppy. Styles go in and out. Like reading a book where every other paragraph is written by a different person, and none of them talked to each other. Sometimes you get PascalCase, sometimes camel_case, sometimes snakeCase. Sometimes booleans are real bools, sometimes they're "Y" "N" (yes, real) or sometimes they're ints. Sometimes functions take in a lot of arguments, sometimes they take in a struct with options. Sometimes missing data is nullable, other times it's an empty string, othertimes its "N/A". And on and on.

You can enforce a lot of this through automatic linters, but not all. You require a set standard and the PR procedure can enforce the standard.