In my opinion this is much, much better than trying to decipher what the heck that one co-worker pushed to production and how exactly it broke things. Of course there are ways to smuggle rubbish through code review, but it's still better than no reviews.
Both have problems. The best system is one where a passing review means "I feel comfortable maintaining this;" too often a passing review means "this is how I would have written it."
I have suffered under far too many "this is how I would have written it" reviews. It can be really demoralizing to write perfectly good, working, readable, performant, well-tested code only to have it rejected and have to rewrite it because the reviewer wishes you had used a different C++ feature.
I tried something like your "I feel comfortable maintaining this" approach recently when I was the reviewer. We have an internal geodata visualization tool that I've done some work on lately, and a talented C++ developer added a cool and useful new feature to the JavaScript front end.
He asked me to review it and said, "I'm sure you will find a lot of things I should change! This is my first real JavaScript project other than some hobby stuff."
And he was right. There was a lot of stuff I would have done differently. There was a mix of jQuery and document.getElementById, a fair amount of repeated code, var instead of const or let, and so on.
Our "house style" for reviews would have been for me to comment on every single one of these things line by line and expect him to change them all to my satisfaction. It would then require a second review pass to clear up misunderstandings, and a third to see if it all ended up OK.
Instead I told him:
"Your new feature is awesome! You're right, there are a lot of nitpicky things I would change in the code to bring it up to more modern JS style, and I'm glad you asked about that. But I didn't find anything that looks broken or dangerous. And the feature works, right? I could spend half a day writing comments explaining everything I would do differently, and that would keep you busy for another half day fixing it up. But I know you have more important things to work on right now, so maybe we can try something different. Go ahead and submit the code as is so people can start using it. When I get a little time I will just go ahead and make the changes I'm thinking of. I'll add review comments on my own changes and send it to you for review so you can see what I changed and why. That will avoid a lot of back-and-forth. I think it will save us both a lot of time and be a more pleasant experience too."
Needless to say, the developer liked this idea. And his manager was within earshot when we discussed it and thanked me for thinking of this approach.
I still haven't made that update, and this reminds me to do it sometime soon. But the code still works, people are using it productively, it has not failed once, and who really cares if there are some minor imperfections in the code style? None of our code is perfect!
You'll never do that update right? The more you wait, the more that developer will get annoyed when you interrupt them with it. :)
In this case actually I would have taken the half an hour and sat with them to explain what I think should be done differently and why, prioritized by importance.
Reviews done through tools work if there aren't many comments and if the basic design is ok.
I've seen reviews where someone's not skilled enough and they have to be taught idioms in dozens of comments in 10-20 patch sets. It's pretty horrible, but the positive side is that at least they're not merging crap.
I resemble that remark! Yes, procrastinator here. ;-)
The thing is, his code is plenty good for now. Even if I never make that update, the code works and people are getting useful results from it. There's nothing overly complicated about it either; I or anyone else could pick it up and easily make any simplifications and improvements whenever needed.
And this developer is working on far more important things for the company, mostly in C++. I made the call that it would be better for our business to let him get back to that, since any improvements to the JavaScript code style on this internal tool simply weren't that urgent.
But I do appreciate the reminder and I will get to that update soon!
That sounds very pragmatic. I probably would have suggested sitting down and making the changes together. However anything that avoids a couple of cycles of review is a bonus, it's super corrosive to productivity and often moral.
Few things to consider:
- linters to help move away from code style comments
- Peer code reviews (if possible)
- inform your team/boss of bike shedding
- use a tool like reviewable.io where comments can be marked as blocking or not. I’ll often comment on preferences and block on issues.
