> Sure, but not everyone agrees on what's "readable" or "maintainable", and no organization has hashed out all the design patterns and coding style that it would deem appropriate. Even if there's a "better" way of doing something, not everyone might agree that it's worth the effort. A lot of "elegant" patterns are less readable. (e.g. object-oriented abstractions vs a functional approach)
This doesn't really matter. Yes, there will always be some level of disagreement. But you aren't writing code in isolation, and it is your job to write code that your team can maintain. If I, a reviewer (who is presumably well versed in the language in question) cannot understand what a block of code is doing without expending significant mental energy, that is a red flag.
I'm talking about refactorings on the orders of 10s of lines of code, not changes to design patterns of architectures, those should be decided much earlier (and in general PRs should be small enough that you aren't forced to redo a design as part of a code review).
> If a reasonable person can understand the code in front of them, and there's nothing objectively wrong with the code,
And this is where we disagree. A reasonable person being able to understand the code is only the starting point. Code should only be merged when, to a good approximation, you're comfortable with anyone on your team (or not on your team, consider onboarding) being able to understand the code quickly.[0]
One pet peeve of mine in this regard is loops that break or continue, especially those that are nested. They're often easy to write, and sure I can figure out what's going on eventually, but it's not easy to maintain, and there are straightforward, mostly mechanical changes that can make things easier to follow:
a loop with a break is almost always an attempt to find something in a container. If you're continuing, it means you've got a function that finds and then does something with the found object. Split it in two. Have a function that loops through and returns the result, and then a second function that processes that result.
A loop with a continue can almost always be refactored into a filter (in practice, I am not saying you should be using map/filter everywhere) by reversing the conditional.
There are exceptions to this: early breaks/guards (sometimes), "returning" by reference in C, and sometimes building state machines can be done most clearly with continues. But those are fairly context specific (and they aren't the context I work in, for the most part), so can be called out explicitly. That's the kind of thing I look at.
[0]: Another way of thinking about this or justifying this: If someone gets paged in the middle of the night because there is some production issue happening, and the trace happens to cross through this chunk of code, will it significantly delay this groggy person, who is neither you nor the author, and who has perhaps never looked at this code before, from quickly isolating the root cause of the issue?
So much of this is subjective. For example, I find break/continue to be far clearer than anonymous functions and map/filter (because you can skip/bail early and for single conditions).
Generally for code review, I find the best strategy isn't "this could be better", and _definitely_ not "I would have done this differently". Currently, I think it's "this gets the job done (is tested, passes acceptance tests), isn't overly clever, and looks like most of our other code". Anything more than that should either be a larger refactoring/rearchitecting job, or threatens to quagmire you in a tarpit of subjectivity.
> So much of this is subjective. For example, I find break/continue to be far clearer than anonymous functions and map/filter (because you can skip/bail early and for single conditions).
To be clear, I am not suggesting anonymous functions, nor am I suggesting using the map/filter constructs. I'm suggesting that
def my_process_seq(seq)
for obj in seq:
if meets(obj, cond):
break
return process(obj)
is better as
def get_obj(seq):
for obj in seq:
if meets(obj, cond):
return obj
process(get_obj(seq))
This gets even more helpful if you have complex conditions, or nested loops and apply this hoisting operation at each level. I have even stronger opinions about how to do this the "best" way. But break and continue are (at least in my line of work) generally the worst way.
This doesn't really matter. Yes, there will always be some level of disagreement. But you aren't writing code in isolation, and it is your job to write code that your team can maintain. If I, a reviewer (who is presumably well versed in the language in question) cannot understand what a block of code is doing without expending significant mental energy, that is a red flag.
I'm talking about refactorings on the orders of 10s of lines of code, not changes to design patterns of architectures, those should be decided much earlier (and in general PRs should be small enough that you aren't forced to redo a design as part of a code review).
> If a reasonable person can understand the code in front of them, and there's nothing objectively wrong with the code,
And this is where we disagree. A reasonable person being able to understand the code is only the starting point. Code should only be merged when, to a good approximation, you're comfortable with anyone on your team (or not on your team, consider onboarding) being able to understand the code quickly.[0]
One pet peeve of mine in this regard is loops that break or continue, especially those that are nested. They're often easy to write, and sure I can figure out what's going on eventually, but it's not easy to maintain, and there are straightforward, mostly mechanical changes that can make things easier to follow:
a loop with a break is almost always an attempt to find something in a container. If you're continuing, it means you've got a function that finds and then does something with the found object. Split it in two. Have a function that loops through and returns the result, and then a second function that processes that result.
A loop with a continue can almost always be refactored into a filter (in practice, I am not saying you should be using map/filter everywhere) by reversing the conditional.
There are exceptions to this: early breaks/guards (sometimes), "returning" by reference in C, and sometimes building state machines can be done most clearly with continues. But those are fairly context specific (and they aren't the context I work in, for the most part), so can be called out explicitly. That's the kind of thing I look at.
[0]: Another way of thinking about this or justifying this: If someone gets paged in the middle of the night because there is some production issue happening, and the trace happens to cross through this chunk of code, will it significantly delay this groggy person, who is neither you nor the author, and who has perhaps never looked at this code before, from quickly isolating the root cause of the issue?