3 ms·

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 o

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.
  - davvolun 7y ago
    > if the code is trying to do a single thing What is a single thing? Login? Initialize db? One line of code, 3 lines of code, ... If you have 100 lines of code and 6 nestings, even if none of that is or, as far as you can tell, should ever be re-used, you should break that into smaller, digestible chunks of DoSubThingA, etc. It's kind of an aside, but you mentioned e.g. `DoThingD` got changed or "some other dev WILL come along" -- if some other dev comes along and doesn't bother to check where and in what context `DoThingD` is being used, and changes it in a way that breaks `DoThingC` or `DoThingE`, they aren't doing their job. Particularly, if `DoThingD` wasn't written to be re-used, and they just use re-write it and use, particularly in a way that breaks its original invariance... I would be having strong words with that developer.
dgb23 7y ago
I understand what you mean. I think a nice middleground between our preferences would be scoped blocks with explicitly passed vars. This would keep the readability you want but still have the other benefits I mentioned. Plus it would be simpler to refactor when needed.
- horsawlarway 7y ago
  This is a good point, and nicely matches a pattern I find myself using in JS where I just define the sub methods inline in the larger method. You get the niceties of a name for the folks who want it, but the code is still grouped nicely and it's easy to walk through the whole operation. I see this much less often in languages where scoping is less flexible though.
oblio 7y ago
I kind of agree with you. If fold markers were universal, I'd say that they would have been the best of both worlds. Longer linear functions with logical blocks foldable. Folds get a bad rap because they aren't that widespread, outside of Emacs/Vim and amusingly, the .NET world, and because some people abuse them to write God-classes and such.

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.
- davvolun 7y ago
  > if the code is trying to do a single thing What is a single thing? Login? Initialize db? One line of code, 3 lines of code, ... If you have 100 lines of code and 6 nestings, even if none of that is or, as far as you can tell, should ever be re-used, you should break that into smaller, digestible chunks of DoSubThingA, etc. It's kind of an aside, but you mentioned e.g. `DoThingD` got changed or "some other dev WILL come along" -- if some other dev comes along and doesn't bother to check where and in what context `DoThingD` is being used, and changes it in a way that breaks `DoThingC` or `DoThingE`, they aren't doing their job. Particularly, if `DoThingD` wasn't written to be re-used, and they just use re-write it and use, particularly in a way that breaks its original invariance... I would be having strong words with that developer.

dgb23 7y ago

I understand what you mean. I think a nice middleground between our preferences would be scoped blocks with explicitly passed vars. This would keep the readability you want but still have the other benefits I mentioned. Plus it would be simpler to refactor when needed.

horsawlarway 7y ago
This is a good point, and nicely matches a pattern I find myself using in JS where I just define the sub methods inline in the larger method. You get the niceties of a name for the folks who want it, but the code is still grouped nicely and it's easy to walk through the whole operation. I see this much less often in languages where scoping is less flexible though.

oblio 7y ago

I kind of agree with you. If fold markers were universal, I'd say that they would have been the best of both worlds. Longer linear functions with logical blocks foldable. Folds get a bad rap because they aren't that widespread, outside of Emacs/Vim and amusingly, the .NET world, and because some people abuse them to write God-classes and such.