Catalog of Elixir-specific code smells

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.

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

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.

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.

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

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

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.

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.

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.

> Labeling what they did as 'smelly' seems impolite and unnecessarily judgemental.

I and my colleague follow a simple rule: Be mean to the code but friendly to the person.

People make mistakes, are in a hurry, don't know any better, sure. But that doesn't make the code any less smelly, buggy, crappy or hard to work with. It's much more productive and even fun to just call it what it is. But this only works in an environment where people are comfortable with each other, take responsibility and want to pragmatically improve and write better code.

Smells can be good too, like freshly cut cedar wood or rain on asphalt. It is interesting that you first go to the terrible stinky smells of shoveling shit and that it sparks such a visceral response. Of course in this scenario, we are referring to less desirable smells, but you really took it to the next level!

What word would you use instead?

I think “smell” was not intended as a value judgement, but as a way to emphasize that there’s probably something wrong nearby, even though it’s hard to articulate.

It annoys me too for similar reasons, but I think it's a lost cause for the moment. For reasons that escape me, most in the industry don't have a similar reaction.

It's weird to see "large messages" as a code smell. I'd say it's more of a thing to be aware about if you use any BEAM-based language: that's just how the runtime works.

But, if you're constantly sending large messages between processes, that might be a sign that those two processes are not as independent as you though, and it might be good to think if there's a way to merge them into a single process.

Another option might be see if you can transform those messages into ref-counter binaries, which will make sending messages quite cheap, since you're sending a single reference to the binary, instead of the whole thing. There's a tradeoff here, of course, depending on how expensive it is to transform your data into a binary, as well as increased complexity of garbage collection of these binaries.

I think this concern/code smell section should be about spamming large messages between processes, in which case the solution could be throttling (but not by the receiving process, as it would still flood its mailbox).

The whole language as well as this repo really drips and oozes the word "Enterprise" in CamelCase as all good enterprise programs should be.

Large messages are just the norm.

[Credo](https://hex.pm/packages/credo) comes with a nice set of default checks and it is straightforward to add your own.