I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely.
But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit.
A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will be motivated to understand it (and potentially improve it). By contrast, with code reviews often the reviewer does not have time to really deep dive into the code and will only have a superficial understanding of it.
Oh, also use code quality scanners to keep an eye on tactical code debt.
The most important reason for code review is not to fix the code but rather to transfer knowledge between developers. I've learned a lot of techniques for writing better code from suggestions from my code reviewers, and from reviewing other people's code.
Without code review, when will a junior developer ever learn anything from a senior developer?
It works both ways, too--it makes more developers aware of the others' work, what changes are being made and so forth. So it's not just expertise that gets transferred, but also the current state of the codebase.
In that case, it doesn't matter if you do the code review before or after they merge and deploy. You could read through the diffs in the commit log whenever it is convenient for you, and leave review comments for the other developer on their PR after the fact.
I have been on teams that never review code and teams that always review code, and I can confidently say I never want to work on the former again. That was with a junior team, but your team is going to have new people, if not junior, at some point. People who are familiar with each other's code and have agreed on standards can review code pretty quick. I would rather have it and not need it than need it and not have it.
I've found PRs are usually a waste of time because people focus on such trivial bullshit because they're easy to find.
98% of the review comments I receive hark back to some handwavy explanation about maintainability. But never in any way that could actually cause a bug.
when I run teams my rules with PRs are if it's not a bug, and it's not against the code review guidelines leave it. It's usually not worth the back and forth unless there is a clear mentor/mentee relationship.
One person writes the code. Many people read the code. If something is unclear or feels wrong to the reader, they get priority. Reviewers should leave all sorts of feedback; the response of the author should be to just implement the feedback unless they can clearly articulate why doing so is not a good idea. You're right that "it's not worth the back and forth", but completely wrong on who should be the one keeping their mouth shut.
Usually the feedback is not "I don't understand this" or "I find this unclear" it's "I think it would be clearer if you did it this way instead of this way". Or "if you made this change it would make it more maintainable".
And if everyone followed everyone else's advice you end up with everyone writing code in their reviewers style instead of their own which doesn't seem like a gain.
Not to mention code changes have a cost, very few devs spend as much time testing and thinking about their code when making code changes. So many times you are trading more thought out, better tested code for less tested less thought out code. Because of this I've seen countless bugs introduced from code changes related to trivial PR requests.
If bugs are being introduced, that means the team's testing culture is poor. If someone asks me to make a change in code review, I know that my modifications are fine because the unit tests still pass (and the integration tests, but it's rarer to have to touch those). If I forget to manually run them the build system runs them before my code is let in.
Writing in "the reviewer's style" is preferred because they are the reader. For most things that are "style" there should be explicit style guides; for the things that aren't, the reader's opinion is better every time.
If your team isn't getting useful code review feedback, that's also a team culture problem and your senior eng need to step up and lead by example.
"I don't understand this" is important feedback. At a minimum, more comments are needed, but it probably also signals that variable or method names need improvement.
My experience (15+ years, companies of all shapes and sizes from college shops to FAANG) is that the kind of engineer who feels that code review feedback isn't worth listening or is a waste of time to is detrimental to the team in both the short and long term.
Very few places have enough testing that code modifications are not correlated with bugs. Based on your comment I think you maybe you're working at places that aren't very representative of the industry as a whole.
> the reader's opinion is better every time.
I don't see why the reader's preference is always better. If you like designing for flexibility and I like KISS why is KISS better when you write the code, and flexibility better when I write it?
You act like our profession isn't plagued by flamewars. You see it on here every day with differences of opinion about microservices, design patterns, designing for today vs the future, KISS vs SOLID, how much testing is appropriate, should you focus more on automation vs integration vs unit tests.
When the organizations views on all these matters are aligned and documented such that the code reviews are structured that's great and you get good feedback. I've worked for 10 companies almost none of them were that aligned and organized.
Code review feedback can be very valuable, I've was the champion for code reviews 8 years ago when most organizations were not doing them.
But I think the vast majority of useful code review feedback falls into one of the following categories
1. Spotted a bug, deviation from spec, or a deficiency in the spec
2. A detailed and concrete comment about maintainability (i.e. this method doesn't handle x case, or blows up on y case and that's not documented)
3. This violates a code review guideline
4. Expressing what is confusing such as complex code that needs documentation or poorly named variables
5. Performance critique
Most of the code review feedback I've seen has not fallen into these categories and instead has to do with one persons preferences vs another.
- I'd break this out into a method vs I'd inline this method
- Differences in code grouping by functional vs logical cohesion.
- You should make this more performant vs this isn't going to be a hotspot so build this for readability over performance
So you're advocating for pair programming all of the time? As someone who has to do both pair programming and code reviews quite a bit this past year (mentoring some quite junior developers), I find pair programming much more mentally exhausting. Two hours of that and my mind is pretty much spent for the day.
IME PR reviews are generally a half-assed substitute for pair programming. I don't really enjoy doing pair programming, but can't deny the effectiveness.
I don't read the parent as advocating for pair programming, just that any given part of the code should be understood by at least two people. This is possible to achieve even on a fully asynchronous team.
But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit.
A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will be motivated to understand it (and potentially improve it). By contrast, with code reviews often the reviewer does not have time to really deep dive into the code and will only have a superficial understanding of it.
Oh, also use code quality scanners to keep an eye on tactical code debt.