5 ms·

I very much disagree. The in-lined version can easily be confusing, harder to change, harder to test and even error prone. By splitting code into functions you

by dgb23 7y ago

I very much disagree. The in-lined version can easily be confusing, harder to change, harder to test and even error prone. By splitting code into functions you have a more well defined scope for each of them and they can be understood in isolation. You also get the benefit of getting a quicker overview (if you care to name your functions well) in your calling function (DoAll). Then there is the issue of maintainability. When I have to change something in a big (in-lined) function then I'm much more anxious about the whole context. My instinct is often to refactor it into smaller pieces so I can narrow down the changes and isolate them correctly and exactly from the context. This might very well be a thing of preference?

horsawlarway 7y ago
I suspect this is mostly preference. Personally, I really dislike pulling everything out into small methods - it literally forces you to remember a crap load of names. I find remembering names to be extremely high cognitive load. Worse, the names WILL lie to you. Especially if you weren't the one doing the naming. So not only are you forced to remember a bunch of labels that better fit someone else's mental model, you're still not off the hook for understanding what the code in those tiny methods is doing, and considering how it might impact the task at hand. So now you're stuck with twice the work - Remembering which name goes with which functional piece of the task at hand, and then remembering how that name actually accomplishes the task (the actual code). Worse, because functions are moved out of line, I find it much harder to jump between relevant bits of actual "doing things" code. Basically - The ONLY time I want to split code off into named chunks is when the alternative is copying/pasting code somewhere. I look for code which is getting reused and break that out. The in-lined version is faster to read, faster to understand (and by understand I mean REALLY understand, not some hand-wavey "I trust this name" understand, but as in you actually know the operations and changes to the system that the call will result in) The downside to inline code is you have to actually read it. I find a lot of the folks who really like short methods struggle to parse the language they're working in and fall back on the name without actually understand what the chunk of code does. But I think they're also the folks who have a better memory for names.
- davvolun 7y ago
  Reducing cognitive load by breaking things into digestible pieces can be hard, and at times it's hard to say if option A is better than option B, but it is not preference. If `extractId` is an eight-step process that's only used once, but I step over it and check locals to find that the variable was correct before the call, and wrong after, then I just found my problem. That would be faster than stepping over each step to find the problem -- basically the debugging version of divide and conquer (for finding the solution, optimal would be binary search, but for readability and maintainability, that should not be the goal). Another example is if I have a boolean condition that incorporates extremely complex logic but is only used once, if I assign it to a variable called `nameWasPreviouslySet`, that is easier to read and understand, and if I'm debugging and expect `nameWasPreviouslySet == true` at this point, and it's not, then I've figured out the bug is in setting `nameWasPreviouslySet`. So DRY shouldn't be the only reason to refactor -- refactor for readability also.
  - horsawlarway 7y ago
    Ok, so lets take your example at face value. You have an extremely complex step of code that you only call once. Already, you have invariants that are easy to break. You're assuming it's only called once. That's relatively safe if you're inline, that's bogus as soon you've broken it out into a named function. Some other dev WILL come along and re-use your helpfully broken out code somewhere, and that invariant no longer holds. Even assuming no one else has re-used it, what ensures that the problem is actually in that method? Particularly if that method itself calls out to many small named methods. So you have some complex code that does something, but the "things" it does are call out to other small chunks of code. so now you have DoComplexThings => { DoSubThingA DoSubThingB DoSubThingC DoSubThingD DoSubThingE return } So you're back to binary search - The problem is in there somewhere, but "in there" is actually calls out to many other functions again. Which one is broken? And wait! DoThingD got changed at some point and will now fail if you run it before DoThingC, but there's nothing in the names that tells you that. So you have hidden deps between chunks of code that are all trying to accomplish a single goal. If you're debugging that, you almost certainly have to go through all the code in that method again, except you have to page back and forth in your editor to get the meaningful pieces into view. All so some OTHER dev could give it name that was meaningful to them, but likely not that helpful when you're debugging because the mental model they have of the system resulted in the bug in the first place. --- Now, rant aside - I think we agree more than we disagree, I think good names matter, and I think there's a lot of wiggle room around when a thing should be broken out. But if the code is trying to do a single thing, and it's not re-used, I'll take a single cohesive method over 8 tiny abstractions ANY day.
olejorgenb 7y ago
As long as the sub-procedure don't need 10+ input parameters and 3+ output parameters/return values I can agree. Otherwise it's a sign that the code/logic simply too interrelated to be broken up well.
nikita2206 7y ago
If you’re talking about scopes of variables then if language supports it, the best of both worlds would be to have an inlined version where each block has its own scope so that you can easily track which variables are actually shared among those scopes and which only belong to certain scopes.
shultays 7y ago
Personally I find all those function calls harder to debug/review. I am perfectly capable of interpreting most non-complex small code blocks with a glance. If someone takes that and replaces with a function it usually forces me to step into function to understand what it really does. Function name wont be as meaningful as the actual code. Forcing stuff to sub functions just hiding information away from the people that reads code.
alephr 7y ago
I don't think it's a preference issue. The problem you have when you split up functions is you're making decisions that have consequences in a fairly arbitrary way(not backed by an understanding of the system, just how you feel things should be broken up). On top of arbitrarily pushing the system in directions, you yourself note that you hide the actual context of the code when you split it up. It might make you feel better in the moment but I don't think it's the right response to feel emboldened by making decisions with less context. Premature abstraction is at the heart of a lot of bad design and complexity. With even primitive dev tools you can get a lot of the benefits (grouping, naming) with comments and braces. More could definitely be done on this front but dev tool progress is sadly pretty slow. Going this route you can have organization while not throwing away the all-important context of what the code is actually meant to accomplish.

by dgb23 7y ago

horsawlarway 7y ago

I suspect this is mostly preference. Personally, I really dislike pulling everything out into small methods - it literally forces you to remember a crap load of names. I find remembering names to be extremely high cognitive load. Worse, the names WILL lie to you. Especially if you weren't the one doing the naming. So not only are you forced to remember a bunch of labels that better fit someone else's mental model, you're still not off the hook for understanding what the code in those tiny methods is doing, and considering how it might impact the task at hand. So now you're stuck with twice the work - Remembering which name goes with which functional piece of the task at hand, and then remembering how that name actually accomplishes the task (the actual code). Worse, because functions are moved out of line, I find it much harder to jump between relevant bits of actual "doing things" code. Basically - The ONLY time I want to split code off into named chunks is when the alternative is copying/pasting code somewhere. I look for code which is getting reused and break that out. The in-lined version is faster to read, faster to understand (and by understand I mean REALLY understand, not some hand-wavey "I trust this name" understand, but as in you actually know the operations and changes to the system that the call will result in) The downside to inline code is you have to actually read it. I find a lot of the folks who really like short methods struggle to parse the language they're working in and fall back on the name without actually understand what the chunk of code does. But I think they're also the folks who have a better memory for names.

davvolun 7y ago
Reducing cognitive load by breaking things into digestible pieces can be hard, and at times it's hard to say if option A is better than option B, but it is not preference. If `extractId` is an eight-step process that's only used once, but I step over it and check locals to find that the variable was correct before the call, and wrong after, then I just found my problem. That would be faster than stepping over each step to find the problem -- basically the debugging version of divide and conquer (for finding the solution, optimal would be binary search, but for readability and maintainability, that should not be the goal). Another example is if I have a boolean condition that incorporates extremely complex logic but is only used once, if I assign it to a variable called `nameWasPreviouslySet`, that is easier to read and understand, and if I'm debugging and expect `nameWasPreviouslySet == true` at this point, and it's not, then I've figured out the bug is in setting `nameWasPreviouslySet`. So DRY shouldn't be the only reason to refactor -- refactor for readability also.
- horsawlarway 7y ago
  Ok, so lets take your example at face value. You have an extremely complex step of code that you only call once. Already, you have invariants that are easy to break. You're assuming it's only called once. That's relatively safe if you're inline, that's bogus as soon you've broken it out into a named function. Some other dev WILL come along and re-use your helpfully broken out code somewhere, and that invariant no longer holds. Even assuming no one else has re-used it, what ensures that the problem is actually in that method? Particularly if that method itself calls out to many small named methods. So you have some complex code that does something, but the "things" it does are call out to other small chunks of code. so now you have DoComplexThings => { DoSubThingA DoSubThingB DoSubThingC DoSubThingD DoSubThingE return } So you're back to binary search - The problem is in there somewhere, but "in there" is actually calls out to many other functions again. Which one is broken? And wait! DoThingD got changed at some point and will now fail if you run it before DoThingC, but there's nothing in the names that tells you that. So you have hidden deps between chunks of code that are all trying to accomplish a single goal. If you're debugging that, you almost certainly have to go through all the code in that method again, except you have to page back and forth in your editor to get the meaningful pieces into view. All so some OTHER dev could give it name that was meaningful to them, but likely not that helpful when you're debugging because the mental model they have of the system resulted in the bug in the first place. --- Now, rant aside - I think we agree more than we disagree, I think good names matter, and I think there's a lot of wiggle room around when a thing should be broken out. But if the code is trying to do a single thing, and it's not re-used, I'll take a single cohesive method over 8 tiny abstractions ANY day.

olejorgenb 7y ago

As long as the sub-procedure don't need 10+ input parameters and 3+ output parameters/return values I can agree. Otherwise it's a sign that the code/logic simply too interrelated to be broken up well.

nikita2206 7y ago

If you’re talking about scopes of variables then if language supports it, the best of both worlds would be to have an inlined version where each block has its own scope so that you can easily track which variables are actually shared among those scopes and which only belong to certain scopes.

shultays 7y ago

Personally I find all those function calls harder to debug/review. I am perfectly capable of interpreting most non-complex small code blocks with a glance. If someone takes that and replaces with a function it usually forces me to step into function to understand what it really does. Function name wont be as meaningful as the actual code. Forcing stuff to sub functions just hiding information away from the people that reads code.

alephr 7y ago

I don't think it's a preference issue. The problem you have when you split up functions is you're making decisions that have consequences in a fairly arbitrary way(not backed by an understanding of the system, just how you feel things should be broken up). On top of arbitrarily pushing the system in directions, you yourself note that you hide the actual context of the code when you split it up. It might make you feel better in the moment but I don't think it's the right response to feel emboldened by making decisions with less context. Premature abstraction is at the heart of a lot of bad design and complexity. With even primitive dev tools you can get a lot of the benefits (grouping, naming) with comments and braces. More could definitely be done on this front but dev tool progress is sadly pretty slow. Going this route you can have organization while not throwing away the all-important context of what the code is actually meant to accomplish.