Fortunately we are humans, and professionally trained humans at that, and we can judge readability and comprehensibility of methods through better measures than whether it crosses a boundary of number of lines.
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.
> Fortunately we are humans, and professionally trained humans at that, and we can judge readability and comprehensibility of methods through better measures than whether it crosses a boundary of number of lines.
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.
I think the rule that "functions should be small enough that they should be easily reasoned about" is a reasonable rule, and it makes sense to follow it 95% of the time.
"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.
> The problem with a long function is not if it has N or N+1 lines of code, it's that length code with many branching conditions is prone to be untestable and introduce non trivial bugs. Once you start to refactor, not only is it easier to parse but harder to break. This fact is known for decades now.
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.
I once saw a professional software engineer try to refactor the spaghetti code in a complex bioinfomactics project written in python.
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 a complete failure. That branch was abandoned and development continued off the spaghetti.
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.
There are way better metrics of function complexity, like how many branching points, how many loops, or even just how many levels of indentation.
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.
Clean code nitpickers mistake the map for the territory. The rules are the map, maintainability is the territory. The map is a model of the territory, but the territory always contains more detail, both zones of maintainability not covered by the rules and zones of unmaintainability covered by the rules. The rules are heuristics, and like all heuristics, they have false positives and false negatives. Being a mature developer means knowing the limits of tools including processes, style, and standards. When they nit to the rules and not to the goal, it doesn't contribute, it distracts.
Fortunately we are humans, and professionally trained humans at that, and we can judge readability and comprehensibility of methods through better measures than whether it crosses a boundary of number of lines.
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.
> Fortunately we are humans, and professionally trained humans at that, and we can judge readability and comprehensibility of methods through better measures than whether it crosses a boundary of number of lines.
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.
I think the rule that "functions should be small enough that they should be easily reasoned about" is a reasonable rule, and it makes sense to follow it 95% of the time.
"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.
> "Functions should be 5 lines or less"
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.
> The problem with a long function is not if it has N or N+1 lines of code, it's that length code with many branching conditions is prone to be untestable and introduce non trivial bugs. Once you start to refactor, not only is it easier to parse but harder to break. This fact is known for decades now.
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.
I once saw a professional software engineer try to refactor the spaghetti code in a complex bioinfomactics project written in python.
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 a complete failure. That branch was abandoned and development continued off the spaghetti.
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.
There are way better metrics of function complexity, like how many branching points, how many loops, or even just how many levels of indentation.
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.
Clean code nitpickers mistake the map for the territory. The rules are the map, maintainability is the territory. The map is a model of the territory, but the territory always contains more detail, both zones of maintainability not covered by the rules and zones of unmaintainability covered by the rules. The rules are heuristics, and like all heuristics, they have false positives and false negatives. Being a mature developer means knowing the limits of tools including processes, style, and standards. When they nit to the rules and not to the goal, it doesn't contribute, it distracts.
It sounds like we have opposite programming styles!