Code should be readable. Once code is readable it makes reviewing the rest much easier. Readable code is more maintainable and problems jump out at you.
Stuff other than readability is important, but if you focus on readability it makes the rest of the review go very smoothly.
when confronted, they explained to me as one would a child, that they cared deeply about code quality
Obviously `ctx` is the most readable.
these people sound useless.
Readability as it applies to code is how easy it is to read the code and understand it in relation to how complicated the problem domain is.
If it takes a low effort, then it's high readability. If it takes a high effort, then it's low readability.
Software engineering is a collaborative process. I don't write code for the machine; I write it for my colleagues and my future self to read. Code we write 6 months ago looks like someone else code. Readability is important unless you're convinced that it's prototype/throwaway code.
After readability then I'd look for tests. It's a theme: good tests are also meant to be read as documentation. Beside ensuring correctness, it's also the example my colleagues or future self can refer for the module usage.
Hence, I now try really hard to ignore readability until the end.
I don't know if this partly just hostile developers, different understanding of code. Ie, is code for humans to read, or for machines to execute? Or if it's dealing with feature factory developers that just want to keep adding despite how brittle the code is becoming. I presume the problem is somewhere in between, but to another extent the normalcy of the absurd where it is normal to spend 5 minutes per line of code to understand it, it is normal to havk in features and do manual regression testing, etc..
First pass: Functional correctness
Second pass: Better ways to do the same thing (optional)
Third pass: your favorite nitpicks.
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.
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.
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.
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.
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.