I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code.

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.

> I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code.

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.

> 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.

I am still not convinced that readability comes before functionality.

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.

Trying to understand the functionality of code which is hard to read is also a huge waste of time. You should try to keep the code easy to read even in very early drafts instead of leaving that until the end.

Hard to read code is subjective. I am guessing you are working with professionals and not high school programmers. Most of the code I see from professional programmers is passable enough for me to

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.

Readability has very little to do with styling.

It's about clearly and concisely expressing ideas.

Leave styling to linters. Reviewers should be focusing on how the code is communicating.

It's assumed in your post that readability review is nitpicking. It's actually about making the code idiomatic, consistent, predictable, quickly understandable, and low mental stress while reading. The "nitpicking" parts you referred mostly is about ensuring code consistency and predictability. But there are more to readability than that:

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.

This structure (especially the optionality of the second pass) is how you end up with perfectly readable code iterating over List<T> when a hash map would suffice and be orders of magnitude more performant.

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 perfectly readable code, it's easy to notice performance problems.

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.

Then again, that one is super easy to find and fix if it turns out to actually be problem.

I don't know if I agree that the second pass is optional. I've found that pass is the one I've seen have the most benefit, particularly with newer developers. It serves two purposes when done well.

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.

As a counter argument, it's much easier for a developer to write code that does the right thing when that code is readable.

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.

yes, the strawman of nitpicking has been killed.