5 ms·

I've been doing elixir professionally for 6 years. I realize I'm in the minority, but as someone who _reads_ a lot of code, I feel that the strong emphasis (and

by latch 4y ago

I've been doing elixir professionally for 6 years. I realize I'm in the minority, but as someone who _reads_ a lot of code, I feel that the strong emphasis (and in some cases, requirement) to extract functions seriously hurts readability. Quickly glancing through this, I see the Complex Branching case as an example (https://github.com/lucasvegi/Elixir-Code-Smells#complex-branching https://github.com/lucasvegi/Elixir-Code-Smells#complex-bran...). It makes my brain need to act like the runtime, storing the stack of the current function in my head, jumping to the function call, then returning back to the original function and restoring the stack (to be clear, I'm not talking about the actual execution, I'm talking about what my brain goes through reading this). In a complex codebase, the jumping can be far (physically and figuratively), especially since there's a strong preference for pattern matching in function parameters (so in the given example, there might be 4 `success_api_response/2`. Inline code is _often_ easier to read and reason about, even when you aren't concerned about a function mutating a parameter (as in the case with elixir).

emerongi 4y ago
I'm working through a new codebase with a lot of code jumping and it can be tough to get an immediate picture of all the effects of a module. In my head, I am sort of copy-pasting code from one module to another to get the full picture. Sometimes I wish editors had a feature to render an inline view when requested, however that is probably a close-to-impossible feature to add.
waynesonfire 4y ago
I agree in so far that creating those tiny functions doesn't help with eliminating the issue the author described. Namely, that a small typo can cause havoc. I assume they were talking about the case statement logic. If you're dealing with a bunch of cases statements conditioning off various fields and also taking into account order, and maybe even logic of what combination of valid field values may mean, could be a recipe for disaster when it's time to start mucking with it. Creating a function per case is not going to help you deal with this. With that said, I don't have a problem with small functions. If named properly can serve as documention for code readers, can hide and limit coupling of implementing details, ect. > Inline code is _often_ easier to read and reason Until it's not. And I don't understand when that happens but I know when it does.
- shikoba 4y ago
  > Until it's not. And I don't understand when that happens but I know when it does. It's very easy, as soon as the number of lines increase. I fix the arbitrary limit that a function should fit into a terminal to 72x24. Take your own limit.
  - waynesonfire 4y ago
    Sure why not! Love it. Interesting, as I thought more about this, I noticed that I evaluate whether creating a separate function would create a too large of cognitive load in jumping around, and if so, I'd inline, even if it exceeds the 72x24 rule. It's such an art. It's a sign of a master craftsman, when one knows when to break the rules.
josevalim 4y ago
I recommend you to open up an issue as the goal of the repository is to have such discussions! I think the biggest question, in this particular code smell, is where to draw the line. The sample function is definitely small enough to justify it but at some point the smell will kick in. So perhaps the smell needs a clarification? Or perhaps it should be grouped alongside a series of "long ***" smells which make it clear the issue is length.
cttet 4y ago
I think this depends on person. There are two levels of reading code, one is getting a rough idea of how the code flows and works, the other is to check all the details of the code. Some people are better at the prior and some are better at later. What this refactoring essentially do is to segment code in natural language blocks. For people who process things in natural language concept blocks this makes it easier to get the general idea of how the function works at a glance and diving in the implementation of the inner functions later. This will of course introduce the overhead that you have mentioned that will increase the time for you to process the detailed flow of code. But as far as I can see it is only an overhead and by spending some time (or smart IDE or a pencil) there will not be a fundamental problem. For people that are good at reading raw code without the help of concept-based segmentation, they may not experience the benefit but only the overhead.
- kamray23 4y ago
  Yeah, this refactoring is dubious but I think it's still correct. "The successful response with `body`" is a lot simpler in my head to comprehend than `{:ok, body}`. There is little to no need to actually know what is being sent, if the types align it is correct. What's actually being sent can then be checked separately in total isolation to be correct. 7±3 things, that's the usual rule for how many variables you can track. Make it any more complex and you'll run into issues with comprehension and by extension with hidden bugs.
AlchemistCamp 4y ago
I agree. If I'm jumping into unfamiliar code or code I haven't read in a while, I strongly prefer to see what gets pattern-matched out of the incoming struct in as few places as possible. Jumping back and forth between a lot of small helper functions that could have been lines in a pattern match is rough. In this specific case, I suspect the x_error_api_response would end up becoming half a dozen different functions and understanding what get_customer returned based on the Env struct would require quite a few trips back and forth. It's a much smaller nit, but I also found the fix for "complex extraction clauses" slightly less readable than the original: https://github.com/lucasvegi/Elixir-Code-Smells#complex-extraction-in-clauses https://github.com/lucasvegi/Elixir-Code-Smells#complex-extr...
kamray23 4y ago
That's certainly a concern. The human brain can only track 7±3 things at once, which can make life very hard for some programmers if a function jumps away often. However, in a well designed functional system, this shouldn't be an issue. You're not supposed to jump into other functions, those functions should be labels for values. Reading `success_api_response(body)` should make you think "the API response for a successful request with `body`, not `{:ok, body}`. Checking the type should only reinforce this view. This is the very nature of good abstractions in a language such as Elixir. In comparison, a badly designed system of abstractions is one in which you must jump to other functions to understand what they do. If `f` is used as anything but a general function parameter, naming a function `update` with no clear reference to what is being updated, other overly generic names such as `calculate` or `execute` with input types that arent `RPNString` or `Process`.
andy_ppp 4y ago
I actually agree with this, the matching is cool for some cases but once you start matching layers deep it becomes unreadable and you need a stack in your head to remember what everything is doing. Sometimes code that reads from top to bottom with a few different cases can be easier to weave your mind through.
dgb23 4y ago
John Carmack has written about it and agrees with you with the caveat that functional programming alleviates or even eliminates this kind of issue. AKA if your extracted procedures are just functions, then it can improve your code. And since this is Elixir we are looking at presumably mostly functional code. http://number-none.com/blow/blog/programming/2014/09/26/carmack-on-inlined-code.html http://number-none.com/blow/blog/programming/2014/09/26/carm... I want to add a third condition that makes this decision easier IMO, because I think it is insufficient to say "if it is a pure function you can extract it". It can often be the case, but it can also lead to what you describe regardless. It is insufficient if the extracted function is just a one-off helper. The remaining downstream code should read as an _abstracted_ form. Meaning the naming of the functions is clear, uniform and in the best cases they represent a domain layer: A mini-language that communicates clearly what is going on, but draws away from the details. And most importantly it should follow John Ousterhout's proposed rule about interface surface to implementation depth ratio, which should be kept as small as possible. To the example: I don't think the amount of cases handled should have any implication on extracting functions here (as mentioned in the example). It would still be clearer to just return the data directly. Nothing is gained until the body of the functions themselves have enough _depth_ to provide a useful abstraction, the function names would naturally fall out of what they provide instead of just being a different way to communicate their body contents.
Sinidir 4y ago
I always wondered how hard it would be to have a simple inline feature for your editor to expand small function code on demand. 1. Statically for known function calls 2. For dynamic dispatch /pattern matching, run a test , record the actually executed code and then inline that.

by latch 4y ago

emerongi 4y ago

I'm working through a new codebase with a lot of code jumping and it can be tough to get an immediate picture of all the effects of a module. In my head, I am sort of copy-pasting code from one module to another to get the full picture. Sometimes I wish editors had a feature to render an inline view when requested, however that is probably a close-to-impossible feature to add.

waynesonfire 4y ago

I agree in so far that creating those tiny functions doesn't help with eliminating the issue the author described. Namely, that a small typo can cause havoc. I assume they were talking about the case statement logic. If you're dealing with a bunch of cases statements conditioning off various fields and also taking into account order, and maybe even logic of what combination of valid field values may mean, could be a recipe for disaster when it's time to start mucking with it. Creating a function per case is not going to help you deal with this. With that said, I don't have a problem with small functions. If named properly can serve as documention for code readers, can hide and limit coupling of implementing details, ect. > Inline code is _often_ easier to read and reason Until it's not. And I don't understand when that happens but I know when it does.

shikoba 4y ago
> Until it's not. And I don't understand when that happens but I know when it does. It's very easy, as soon as the number of lines increase. I fix the arbitrary limit that a function should fit into a terminal to 72x24. Take your own limit.
- waynesonfire 4y ago
  Sure why not! Love it. Interesting, as I thought more about this, I noticed that I evaluate whether creating a separate function would create a too large of cognitive load in jumping around, and if so, I'd inline, even if it exceeds the 72x24 rule. It's such an art. It's a sign of a master craftsman, when one knows when to break the rules.

josevalim 4y ago

I recommend you to open up an issue as the goal of the repository is to have such discussions! I think the biggest question, in this particular code smell, is where to draw the line. The sample function is definitely small enough to justify it but at some point the smell will kick in. So perhaps the smell needs a clarification? Or perhaps it should be grouped alongside a series of "long ***" smells which make it clear the issue is length.

cttet 4y ago

I think this depends on person. There are two levels of reading code, one is getting a rough idea of how the code flows and works, the other is to check all the details of the code. Some people are better at the prior and some are better at later. What this refactoring essentially do is to segment code in natural language blocks. For people who process things in natural language concept blocks this makes it easier to get the general idea of how the function works at a glance and diving in the implementation of the inner functions later. This will of course introduce the overhead that you have mentioned that will increase the time for you to process the detailed flow of code. But as far as I can see it is only an overhead and by spending some time (or smart IDE or a pencil) there will not be a fundamental problem. For people that are good at reading raw code without the help of concept-based segmentation, they may not experience the benefit but only the overhead.

kamray23 4y ago
Yeah, this refactoring is dubious but I think it's still correct. "The successful response with `body`" is a lot simpler in my head to comprehend than `{:ok, body}`. There is little to no need to actually know what is being sent, if the types align it is correct. What's actually being sent can then be checked separately in total isolation to be correct. 7±3 things, that's the usual rule for how many variables you can track. Make it any more complex and you'll run into issues with comprehension and by extension with hidden bugs.

AlchemistCamp 4y ago

I agree. If I'm jumping into unfamiliar code or code I haven't read in a while, I strongly prefer to see what gets pattern-matched out of the incoming struct in as few places as possible. Jumping back and forth between a lot of small helper functions that could have been lines in a pattern match is rough. In this specific case, I suspect the x_error_api_response would end up becoming half a dozen different functions and understanding what get_customer returned based on the Env struct would require quite a few trips back and forth. It's a much smaller nit, but I also found the fix for "complex extraction clauses" slightly less readable than the original: https://github.com/lucasvegi/Elixir-Code-Smells#complex-extraction-in-clauses https://github.com/lucasvegi/Elixir-Code-Smells#complex-extr...

kamray23 4y ago

That's certainly a concern. The human brain can only track 7±3 things at once, which can make life very hard for some programmers if a function jumps away often. However, in a well designed functional system, this shouldn't be an issue. You're not supposed to jump into other functions, those functions should be labels for values. Reading `success_api_response(body)` should make you think "the API response for a successful request with `body`, not `{:ok, body}`. Checking the type should only reinforce this view. This is the very nature of good abstractions in a language such as Elixir. In comparison, a badly designed system of abstractions is one in which you must jump to other functions to understand what they do. If `f` is used as anything but a general function parameter, naming a function `update` with no clear reference to what is being updated, other overly generic names such as `calculate` or `execute` with input types that arent `RPNString` or `Process`.

andy_ppp 4y ago

I actually agree with this, the matching is cool for some cases but once you start matching layers deep it becomes unreadable and you need a stack in your head to remember what everything is doing. Sometimes code that reads from top to bottom with a few different cases can be easier to weave your mind through.

dgb23 4y ago

John Carmack has written about it and agrees with you with the caveat that functional programming alleviates or even eliminates this kind of issue. AKA if your extracted procedures are just functions, then it can improve your code. And since this is Elixir we are looking at presumably mostly functional code. http://number-none.com/blow/blog/programming/2014/09/26/carmack-on-inlined-code.html http://number-none.com/blow/blog/programming/2014/09/26/carm... I want to add a third condition that makes this decision easier IMO, because I think it is insufficient to say "if it is a pure function you can extract it". It can often be the case, but it can also lead to what you describe regardless. It is insufficient if the extracted function is just a one-off helper. The remaining downstream code should read as an _abstracted_ form. Meaning the naming of the functions is clear, uniform and in the best cases they represent a domain layer: A mini-language that communicates clearly what is going on, but draws away from the details. And most importantly it should follow John Ousterhout's proposed rule about interface surface to implementation depth ratio, which should be kept as small as possible. To the example: I don't think the amount of cases handled should have any implication on extracting functions here (as mentioned in the example). It would still be clearer to just return the data directly. Nothing is gained until the body of the functions themselves have enough _depth_ to provide a useful abstraction, the function names would naturally fall out of what they provide instead of just being a different way to communicate their body contents.

Sinidir 4y ago

I always wondered how hard it would be to have a simple inline feature for your editor to expand small function code on demand. 1. Statically for known function calls 2. For dynamic dispatch /pattern matching, run a test , record the actually executed code and then inline that.