4 ms·

Fortunately we are humans, and professionally trained humans at that, and we can judge readability and comprehensibility of methods through better measures than

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.

rbanffy 2mo ago
> 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.
- ryanbrunner 2mo ago
  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.
  - rbanffy 2mo ago
    > "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.
    - ryanbrunner 2mo ago
      Sorry, you're right - 20 lines or less is the recommendation in the actual text of Clean Code. I have seen people try to push that down to 5 in Ruby on Rails development (IIRC, the most popular linter at one time tried to enforce 5 lines) In any case, the point stands - there are 19 line functions that are too sense, and 21 line functions that are sensible.
      - rbanffy 2mo ago
        A hard boundary should only emit a warning. The same with nested loops and ifs.
- josephg 2mo ago
  I think it’s very easy to over apply that rule, and break a large function into a lot of small functions that you need to scroll up and down to reason about. Here’s a 150 line long function I wrote which I think is quite beautiful: https://github.com/josephg/diamond-types/blob/e143890a596aafdd7ba3e7ae25f9f3749f45acff/src/causalgraph/graph/tools.rs#L355 https://github.com/josephg/diamond-types/blob/e143890a596aaf... This function traverses a DAG given 2 points in the dag, A and B. It breaks the dag into 4 regions - the nodes which are (transitively) only in the parent subgraph of A, B, in both or - implicitly - in neither. It runs in O(n log n) time. How would you improve this function? It could use a better doc comment. But do you honestly think it would be better if it were broken into a lot of small functions, each called once? How would you do it?
locknitpicker 2mo ago
> 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. Lines or code is an indicator, not a goal. If you write long-winded functions, your code is bug prone and harder to test and verify. If you refactor it, it gets shorter. Where do you draw the line? The same goes for how many characters you accept between two line breaks. Some go for 76. Some for 130 or more. There is no difference if your line has 129 or 131 chatacters, but if you spew a comment with 999 characters in a single line then your feedback is actionable if you say "hey man, don't be that guy. Rewrite your comment and make it readable." > (...) PR reviews would be better by actually judging things like readability directly rather than relying on measures that estimate those qualities. Not really. Calling out basic things like "this function is far too long" is clear, objective, and actionable feedback. That is a good PR comment. Dismissing clear and actionable feedback as some guys whims is a red flag, and a telltale sign of someone who has no interest to improve their output and address issues.
- ryanbrunner 2mo ago
  I think what I intended to get across isn't too incompatible with what you're saying. I think it's fine to say "this function is too long", and long functions generally speaking should be something that's worthy of a code review comment. I think the issue that originally launched this thread of the discussion is people who take the specific rules to a dogmatic level and apply rules blindly, and transform "functions should not be long" to "functions should be less than 20 lines no matter what". 20 lines (or whatever standard you land on) should be a guideline and not an inviolable law of the universe, and if reducing a method below 20 lines harms comprehensibility and readability, you're letting the guideline get in the way of the actual goal.

rbanffy 2mo ago

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

ryanbrunner 2mo ago
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.
- rbanffy 2mo ago
  > "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.
  - ryanbrunner 2mo ago
    Sorry, you're right - 20 lines or less is the recommendation in the actual text of Clean Code. I have seen people try to push that down to 5 in Ruby on Rails development (IIRC, the most popular linter at one time tried to enforce 5 lines) In any case, the point stands - there are 19 line functions that are too sense, and 21 line functions that are sensible.
    - rbanffy 2mo ago
      A hard boundary should only emit a warning. The same with nested loops and ifs.
josephg 2mo ago
I think it’s very easy to over apply that rule, and break a large function into a lot of small functions that you need to scroll up and down to reason about. Here’s a 150 line long function I wrote which I think is quite beautiful: https://github.com/josephg/diamond-types/blob/e143890a596aafdd7ba3e7ae25f9f3749f45acff/src/causalgraph/graph/tools.rs#L355 https://github.com/josephg/diamond-types/blob/e143890a596aaf... This function traverses a DAG given 2 points in the dag, A and B. It breaks the dag into 4 regions - the nodes which are (transitively) only in the parent subgraph of A, B, in both or - implicitly - in neither. It runs in O(n log n) time. How would you improve this function? It could use a better doc comment. But do you honestly think it would be better if it were broken into a lot of small functions, each called once? How would you do it?

locknitpicker 2mo ago

> 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. Lines or code is an indicator, not a goal. If you write long-winded functions, your code is bug prone and harder to test and verify. If you refactor it, it gets shorter. Where do you draw the line? The same goes for how many characters you accept between two line breaks. Some go for 76. Some for 130 or more. There is no difference if your line has 129 or 131 chatacters, but if you spew a comment with 999 characters in a single line then your feedback is actionable if you say "hey man, don't be that guy. Rewrite your comment and make it readable." > (...) PR reviews would be better by actually judging things like readability directly rather than relying on measures that estimate those qualities. Not really. Calling out basic things like "this function is far too long" is clear, objective, and actionable feedback. That is a good PR comment. Dismissing clear and actionable feedback as some guys whims is a red flag, and a telltale sign of someone who has no interest to improve their output and address issues.

ryanbrunner 2mo ago
I think what I intended to get across isn't too incompatible with what you're saying. I think it's fine to say "this function is too long", and long functions generally speaking should be something that's worthy of a code review comment. I think the issue that originally launched this thread of the discussion is people who take the specific rules to a dogmatic level and apply rules blindly, and transform "functions should not be long" to "functions should be less than 20 lines no matter what". 20 lines (or whatever standard you land on) should be a guideline and not an inviolable law of the universe, and if reducing a method below 20 lines harms comprehensibility and readability, you're letting the guideline get in the way of the actual goal.