There is absolutely a place for PR reviews, and I don't think the person you were replying to was against that, just that PR reviews would be better by actually judging things like readability directly rather than relying on measures that estimate those qualities.
I can think of many times arbitrary rules like linting or Clean Code-esque standards resulted in a "solution" of making my code less readable.
It's very hard to make a function you need to scroll back and forth to understand readable. Break it into smaller ideas that are more easily reasoned about. We love to think we are too clever, but we are not and we always need to keep an eye on cognitive load - having epifanies when you finally understand how something works is a great feeling, but relying on epifanies coming to you when you are trying to figure out how something works because it's not working now, is a terrible practice.
"Functions should be 5 lines or less" is a measure that approximates that rule, but isn't exactly the same thing - I hope you agree we could both come up with 4 line functions that are impossibly complex or 6 line functions that are easily reasoned about.
I think with Clean Code (and a lot of these kinds of things - Design Patterns is a great old example of this), people can get too dogmatic about applying these sort of approximated rules, when it would make a lot more sense for someone else (i.e. not the code writer) to use their best judgement and just directly answer the question "is this function easy to reason about?" rather than using the approximate measure.
This can’t really be a serious guideline unless you are writing APL, in which 5 lines can already be daunting to grasp. This guidance depends on the language. For Python, once you get over 50 lines it starts to look like you don’t actually know what you are doing anymore.
A function with too many lines is, most likely, doing more than one thing. Functions should do one thing, be easy to test (with few or no external dependencies whenever possible), be deterministic (unless required not to be), and so on. Excessive mocking is another code smell I look for - it often betrays poorly designed functions that can't be easily tested.
It was a complete failure. That branch was abandoned and development continued off the spaghetti.
There is a reason bioinformatics has its own set of viz charts that only they use.
That's my anecdote anyway, it led me to the conclusion that sometimes things are continuous spaghetti and other than some small organizational changes, attempts to exhaustively discretize the code are a fools errand.
The biggest benefits most projects like that are likely to see are performance and debugging improvements accomplished by factoring out recursion.
Mapping to terrain is always the real effort in my opinion.
It was considered too hard, most likely because the present state of the code already degenerated beyond recovery. It might be difficult, but it's never impossible.
> Mapping to terrain is always the real effort in my opinion.
Yes. The domain might be complex, and it might be possible that there are no simple ways to work within that domain. Irreducible complexity is, after all, a thing.
TDD, OOP, Clean Code, etc are an attempt to solve very real problems. They are then applied as dogma to places where these problems are not evident. That's the issue. Of course these rules have their place, but always with a caveat and never applied over all possible places where they might fit. Very often, a better solution exists, as well.