If the code is doing the wrong thing in the first place, no amount of nitpicking on readability will save time.
First pass: Functional correctness
Second pass: Better ways to do the same thing (optional)
Third pass: your favorite nitpicks.
First pass: Functional correctness
Second pass: Better ways to do the same thing (optional)
Third pass: your favorite nitpicks.
So the first priority is getting the change to a readable state. I'm not talking about nits, I'm talking about minimizing the cognitive overhead to truly understand what the code is doing.
That being said, it is also easy to identify nits during this phase. In codebases that have collections of best practices, this often helps to ensure that the change is expressed in terms of a common vocabulary. Once this pass is done, the reviewer can usually better understand the intent of the changes.
There is a spectrum between style differences and absolutely unreadable code.
If it is absolutely unreadable, sure, send it back to be readable. But if it is merely styling issues, I'd encourage you to understand that you are not saving any time by focusing on styling before functionality.
Far more developer time is spent reading code than writing it.
If you can speed up the time required to understand a piece a code by improving the style then it's almost always worth it. For a professional software engineer, just above "absolutely unreadable" is far too low a bar to aim for.
Imagine a developer building a PR for 2 weeks, then reviews back and forth for 2 more weeks. Now 4 weeks have passed and only now the reviewer reviews the functionality - only to find that the entire implementation is wrong/could be done in a better way.
What a waste of 4 weeks of both the author and the reviewer! This could have been short-circuited very early in the process.
On my team, we default to early feedback on functionality and let the CI enforce what it can. Everything else is debatable.
1. Review functionality and then
2. Point out something about coding style.
In fact, if the functionality is good, I even approve the PR leaving a lot of code styling comments that the author can fix at their own leisure.
It's about clearly and concisely expressing ideas.
Leave styling to linters. Reviewers should be focusing on how the code is communicating.
1) did the code use a third party library while a standard lib is sufficient? Is there a way to use native language features (e.g. list comprehension) to make the code more idiomatic to people familiar with the language?
2) are the namings make sense in the business context? Can a new hire read just the public interface and be able to guess the usage?
3) do people have to jump around, full-text-search the code base to understand what's going on? Is automatic dependencies injection being overused? How many things people need to keep in their head to be able to follow this code?
It's all readability there, and I'd argue it's not only about style or formatting.
Nobody can do it perfectly, but I think trying to keep personal nitpicks out of reviews as much as possible - ideally by codifying team nitpicks in automated formatting/.prettierrc/whatever - lets people focus on things that matter: what the code does and how it does it.
In my experience, unreadable code is usually where the worst performance problems are, because that's where they are the hardest to find and fix.
You seem to be conflating readability with formatting, which is not what I mean by readability. Poor and inconsistent formatting adds some friction to reading, but it's a relatively minor part of readability.
First it gives you an idea of how well thought out the implementation was (i.e. was this a quick hack to just finish a asked for requirement) or was a best design targeted. It also helps newer developers develop a voice. Often, at the start, newer devs will take what a more senior dev says as gospel, but by striking up a conversation and, in some ways, making them defend their choices it can help build confidence and that voice to speak up when something doesn't seem right.
Second, I've found it a good way to introduce people to new approaches to accomplishing tasks. Not everyone spends their off hours studying patterns and practices and rarely is there time during a work day to do this properly so code reviews are a natural place to bring these things up as there's concrete comparisons and examples to work with. That helps spark a dev's interest to look in to the topics further.
Too often I see code that is _maybe_ correct, but the reviewer can't actually be sure of it without manually testing it, and the chances that a future reader in 4 months will have any idea what it does or why are extremely low. Good variable and function names get you 90% of the way there yet for some reason cryptic code is still all too common in the world.
To me, reviewing for readability isn't nitpicking - it's equivalent to a mechanical engineering design review pointing out that design for maintenance or design for assembly has been entirely ignored.