How do you successfully nitpick during a code review?

Question

I've been writing code for nearly a decade at various large companies and have seen code quality vary drastically from team to team even within the same company. I like to think that I hold the standard for my code quality to a pretty high standard In in terms of readability, best practices, modularity, and the likes.One of the projects I maintain now has a few people contributing to it, but the majority of the contributions are mine (~90% according to git).When other people submit code for me to review, I am often distracted by the quality of the code in the pull request and find myself requesting tons of changes that have to do with the code quality itself (better variable naming, hiding implementations behind clean interfaces to reduce clutter, breaking large functions into smaller, more readable chunks, etc). This leads to me focusing less on the functionality of the code, but more on the style which wastes a lot of time.This is a frustrating experience both for me, and for the person who is trying to submit code. My intent behind these requests isn't to nitpick, but to hold others to a high standard, and mostly keep the codebase clean so that the new code is easy enough to read for a new person on the team to jump in and contribute.These requested changes are often ignored or are taken as optional suggestions that the person contributing.So two questions: 1. Am I being overly pedantic when it comes to style and readability? 2. And if not, how can I best communicate to my teammates and other contributors that code quality is important and that these "nitpicks" are important?

codeapprove · Accepted Answer

The key to these types of interactions is to make them impersonal. In the case of coding standards (which don't have to do with logic/implementation) try to get all of them enforced by either an automated linter/formatter or a written style guide. Then you're not saying "I think you should ..." it's either a CI error or a link from you to the style guide. There shouldn't be any argument on these points.
Another thing to do is to make sure the person you're reviewing understands how much you care about each comment. I like to prefix things I don't care about with "nit:" or "optional:" to signal that I noticed something and wanted to share my thoughts but I don't think it's a blocking issue.
Finally make sure you're being humble and friendly. If you're worried that a certain block of code might confuse a future reader, say that it confused you! There's a huge emotional difference between "this is confusing, please refactor" vs. "This was hard for me to understand at first, could you consider refactoring or adding a comment to make this easier for future readers?". By showing your own struggles, you can bring more empathy into the conversation.
----
While I'm here ... I should plug that I am the creator of https://codeapprove.com which is a tool designed to help you review code like a pro. Code review is all about reaching distributed consensus, and CodeApprove helps you get there faster.

porbelm · Answer

Focus on getting code standards implemented at your place, like variable naming etc, and if they can't follow that get better coders I guess?

oxff · Answer

I don't really understand how style can be an issue, doesn't the linter / formatter catch like majority of issues or can you not configure one for your project?