6 ms·

Refactoring Large Functions

72 points by muth02446 8y ago

piinbinary 8y ago
It is important to remember that functions exist for the sake of humans, not computers. The computer wouldn't care if all the code was in a giant main() function with GOTOs and a manually-managed stack, the programmers are the ones who have a problem with this.
vincnetas 8y ago
Item number 0 MUST be : Make a copy of current function as old_function and add unit test to check if input on refactored function still produces same results as old_function.
- thanatropism 8y ago
  That's why you have version control and batches and fuzzing etc.
  - vincnetas 8y ago
    Not sure how version control can help here, as unit tests are not run on old versions. About batches, could you be more specific, i might be missing something here. And for fuzzing, it will test your new function, but will not provide any guarantees that your new functions is working exactly like the old one. You might be surprised, bet there could be bugs in old function which can be fixed by refactoring. Sounds nice? But might be some legacy codes is depending on the bug that you fixed, and for backwards compatibility reasons that bug must stay be renamed from "bug" to "functionality".
    - thanatropism 8y ago
      I meant "branches". Mobile devices speak for us, it's an unbearable situation.
  - AlexCoventry 8y ago
    I think the point is to have a unit test like for parameters in parameter_generator(n=100): assert(old_version(*parameters) == new_version(*parameters), f"Versions produced different outputs on {parameters}") This lets you transform old_version with more confidence that you're not corrupting its logic.
dasmoth 8y ago
Do people really prefer: function doFirstBit() {} function doSecondBit() {} function big() { let x,y,z; [x,y] = doFirstBit(); [x,z] = doSecondBit(x, y); } to: function big() { let x,y,z; // First bit... ... // Second bit... ... } To me, it's no harder to find the portion of interest in the latter when making a local change, and substantially easier to understand the whole thing (because you can sit down, maybe with a printout if that's your thing, and read through it linearly without having to jump back and forth to the subfunctions). And passing data between the portions can quickly end up nastier than my contrived example above, at which point people start doing things like creating a class just to hold what used to be the function's stack-frame. Not saying very long functions are something to celebrate. But if they're organised sensibly they're not a huge problem, and to be honest probably bother me less than functions which aren't abstracting anything terribly interesting and only get called once.
- kyberias 8y ago
  Yes, people really prefer. You can get rid of the silly comments, for one thing, and express the idea by naming the functions properly. With a printout? Well, if you allow functions to be so long that you have to print them out, then you probably a lot of other problems with your code base.
- piinbinary 8y ago
  I think that if "First bit," "Second bit," etc can be called out that explicitly, I don't mind big functions that much. The trouble comes from functions where the various bits are all mixed together. This means that comments cannot easily name the sections (for me, one of the main advantages of a function is that it must be given a name), and you cannot easily narrow your reading to just the part you care about. Another advantage of smaller functions is that some of them end up being general enough functionality to be worth sharing with other code. Finally, it's possible to test doFirstBit() independently of the other logic in big().
  - joshuamorton 8y ago
kyberias 8y ago
I've seen my share of code bases and many times I've seen a long function, I've made a number of other observations: The long function has a lot of comments that try to describe the different steps. The comments are often confusing because whoever wrote the comments couldn't quite understand themselves what is happening. There is a significant frequency of bugs found in the functionality implemented or coordinated by that function. The long function has poor code coverage. For me, a long function is usually a code smell.
- kraftman 8y ago
  Yeah often the comments are the names of the inner functions that should be extracted out too.
joemanaco 8y ago
John Carmack on this topic: http://number-none.com/blow/john_carmack_on_inlined_code.html http://number-none.com/blow/john_carmack_on_inlined_code.htm... And I easily agree 100% with him having tried both approaches in bigger projects.
- Groxx 8y ago
  There's a bit of a fuzzy edge in there, where he favors "inline, don't break up artificially" but also "more pure functional code". Breaking pure logic out of a large, otherwise mixed-purity func gives you those stronger purity guarantees / separations, which seems to favor more funcs. But he's also huge on "don't fight the compiler" and static analysis, all of which are aided by strictly pure funcs, so it seems in-character.
  - jungler 8y ago
    That's because he comes from a domain where mutable state is a necessary thing in making the different parts of game logic feed into each other. It is the focus of debugging. The actual computations around what changed can be pure, but turning them into a concurrently operating loop is done most effectively with a static sequencing(and hence, straightline imperative). Or in other terms, he is using a different strategy for different parts of the codebase. At the top level it's imperative, but when you drill down into the callstack, it isn't. A lot of folks work on request-response systems all day, and in those, the necessary state changes tend to be once-per-request and wrapped in transaction or session logic. So it's a lot less crucial to have this kind of strategy for debugging purposes.
- shoo 8y ago
  The anecdote Carmack shares from aviation is interesting: >Indeed, if memory serves (it's been a while since I read about this)... > >The fly-by-wire flight software for the Saab Gripen (a lightweight >fighter) went a step further. It disallowed both subroutine calls and >backward branches, except for the one at the bottom of the main loop. >Control flow went forward only. Sometimes one piece of code had to leave >a note for a later piece telling it what to do, but this worked out well >for testing: all data was allocated statically, and monitoring those >variables gave a clear picture of most everything the software was doing. >The software did only the bare essentials, and of course, they were >serious about thorough ground testing. > >No bug has ever been found in the "released for flight" versions of that >code. > > Henry Spencer
cpeterso 8y ago
The article mentions Steve McConnel's advice on parameter lists (from his great book "Code Complete: A Practical Handbook of Software Construction"), but left out some interesting commentary on function length from the same book: Some studies of code quality from the 1980s reported that defects per KLOC was inversely correlated with function length, leveling off around 200–400 LOC per function. Software composed of many tiny functions is more difficult to understand because there is more context "off screen" to keep in your head. A Cisco study of code reviews found that 200–400 LOC is the limit of how many LOC can be effectively reviewed per hour. Applying the findings of these studies suggests that neither functions nor patches/pull-request diffs should not exceed 200 LOC. FWIW, I have worked on commercial software that had functions many thousands of lines long! :)
sifoobar 8y ago
Some things are better said in one chunk, there are no rules. I often write longer functions while figuring things out, when I need more context in front of me to stay in flow. Then I wait, and wait; until I can't stand it any more, at which point I usually know enough to substantially improve the code. Assuming it survived that long. Refactoring code that you're going to throw away is not very constructive. And the more effort put into making code pretty, the harder it will be to let go when that's the right thing to do. None of this works in a corporate setting, where there's never enough time or money to do anything properly. There are no awesome short term profits to be made from prototyping and keeping code in good health. Corporations also usually prefer rigid rules over competence and intuition, which means whatever cure they come up with will be worse than the disease. And as a result, corporate code is usually crap code.

72 points by muth02446 8y ago

It is important to remember that functions exist for the sake of humans, not computers. The computer wouldn't care if all the code was in a giant main() function with GOTOs and a manually-managed stack, the programmers are the ones who have a problem with this.

vincnetas 8y ago

Item number 0 MUST be : Make a copy of current function as old_function and add unit test to check if input on refactored function still produces same results as old_function.

thanatropism 8y ago
That's why you have version control and batches and fuzzing etc.
- vincnetas 8y ago
  Not sure how version control can help here, as unit tests are not run on old versions. About batches, could you be more specific, i might be missing something here. And for fuzzing, it will test your new function, but will not provide any guarantees that your new functions is working exactly like the old one. You might be surprised, bet there could be bugs in old function which can be fixed by refactoring. Sounds nice? But might be some legacy codes is depending on the bug that you fixed, and for backwards compatibility reasons that bug must stay be renamed from "bug" to "functionality".
  - thanatropism 8y ago
    I meant "branches". Mobile devices speak for us, it's an unbearable situation.
- AlexCoventry 8y ago
  I think the point is to have a unit test like for parameters in parameter_generator(n=100): assert(old_version(*parameters) == new_version(*parameters), f"Versions produced different outputs on {parameters}") This lets you transform old_version with more confidence that you're not corrupting its logic.

dasmoth 8y ago

Do people really prefer: function doFirstBit() {} function doSecondBit() {} function big() { let x,y,z; [x,y] = doFirstBit(); [x,z] = doSecondBit(x, y); } to: function big() { let x,y,z; // First bit... ... // Second bit... ... } To me, it's no harder to find the portion of interest in the latter when making a local change, and substantially easier to understand the whole thing (because you can sit down, maybe with a printout if that's your thing, and read through it linearly without having to jump back and forth to the subfunctions). And passing data between the portions can quickly end up nastier than my contrived example above, at which point people start doing things like creating a class just to hold what used to be the function's stack-frame. Not saying very long functions are something to celebrate. But if they're organised sensibly they're not a huge problem, and to be honest probably bother me less than functions which aren't abstracting anything terribly interesting and only get called once.

kyberias 8y ago
Yes, people really prefer. You can get rid of the silly comments, for one thing, and express the idea by naming the functions properly. With a printout? Well, if you allow functions to be so long that you have to print them out, then you probably a lot of other problems with your code base.
piinbinary 8y ago
I think that if "First bit," "Second bit," etc can be called out that explicitly, I don't mind big functions that much. The trouble comes from functions where the various bits are all mixed together. This means that comments cannot easily name the sections (for me, one of the main advantages of a function is that it must be given a name), and you cannot easily narrow your reading to just the part you care about. Another advantage of smaller functions is that some of them end up being general enough functionality to be worth sharing with other code. Finally, it's possible to test doFirstBit() independently of the other logic in big().
- joshuamorton 8y ago

kyberias 8y ago

I've seen my share of code bases and many times I've seen a long function, I've made a number of other observations: The long function has a lot of comments that try to describe the different steps. The comments are often confusing because whoever wrote the comments couldn't quite understand themselves what is happening. There is a significant frequency of bugs found in the functionality implemented or coordinated by that function. The long function has poor code coverage. For me, a long function is usually a code smell.

kraftman 8y ago
Yeah often the comments are the names of the inner functions that should be extracted out too.

joemanaco 8y ago

John Carmack on this topic: http://number-none.com/blow/john_carmack_on_inlined_code.html http://number-none.com/blow/john_carmack_on_inlined_code.htm... And I easily agree 100% with him having tried both approaches in bigger projects.

Groxx 8y ago
There's a bit of a fuzzy edge in there, where he favors "inline, don't break up artificially" but also "more pure functional code". Breaking pure logic out of a large, otherwise mixed-purity func gives you those stronger purity guarantees / separations, which seems to favor more funcs. But he's also huge on "don't fight the compiler" and static analysis, all of which are aided by strictly pure funcs, so it seems in-character.
- jungler 8y ago
  That's because he comes from a domain where mutable state is a necessary thing in making the different parts of game logic feed into each other. It is the focus of debugging. The actual computations around what changed can be pure, but turning them into a concurrently operating loop is done most effectively with a static sequencing(and hence, straightline imperative). Or in other terms, he is using a different strategy for different parts of the codebase. At the top level it's imperative, but when you drill down into the callstack, it isn't. A lot of folks work on request-response systems all day, and in those, the necessary state changes tend to be once-per-request and wrapped in transaction or session logic. So it's a lot less crucial to have this kind of strategy for debugging purposes.
shoo 8y ago
The anecdote Carmack shares from aviation is interesting: >Indeed, if memory serves (it's been a while since I read about this)... > >The fly-by-wire flight software for the Saab Gripen (a lightweight >fighter) went a step further. It disallowed both subroutine calls and >backward branches, except for the one at the bottom of the main loop. >Control flow went forward only. Sometimes one piece of code had to leave >a note for a later piece telling it what to do, but this worked out well >for testing: all data was allocated statically, and monitoring those >variables gave a clear picture of most everything the software was doing. >The software did only the bare essentials, and of course, they were >serious about thorough ground testing. > >No bug has ever been found in the "released for flight" versions of that >code. > > Henry Spencer

cpeterso 8y ago

The article mentions Steve McConnel's advice on parameter lists (from his great book "Code Complete: A Practical Handbook of Software Construction"), but left out some interesting commentary on function length from the same book: Some studies of code quality from the 1980s reported that defects per KLOC was inversely correlated with function length, leveling off around 200–400 LOC per function. Software composed of many tiny functions is more difficult to understand because there is more context "off screen" to keep in your head. A Cisco study of code reviews found that 200–400 LOC is the limit of how many LOC can be effectively reviewed per hour. Applying the findings of these studies suggests that neither functions nor patches/pull-request diffs should not exceed 200 LOC. FWIW, I have worked on commercial software that had functions many thousands of lines long! :)

sifoobar 8y ago

Some things are better said in one chunk, there are no rules. I often write longer functions while figuring things out, when I need more context in front of me to stay in flow. Then I wait, and wait; until I can't stand it any more, at which point I usually know enough to substantially improve the code. Assuming it survived that long. Refactoring code that you're going to throw away is not very constructive. And the more effort put into making code pretty, the harder it will be to let go when that's the right thing to do. None of this works in a corporate setting, where there's never enough time or money to do anything properly. There are no awesome short term profits to be made from prototyping and keeping code in good health. Corporations also usually prefer rigid rules over competence and intuition, which means whatever cure they come up with will be worse than the disease. And as a result, corporate code is usually crap code.