Catalog of Elixir-specific code smells

https://github.com/lucasvegi/Elixir-Code-Smells

by sbuttgereit • 4 years ago

107 29 4 years ago

And a video talking about the repo: https://www.youtube.com/watch?v=pV6YxGPvx4U&t=94s

latch• 4 years 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-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• 4 years 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.

waynesonfire2• 4 years 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• 4 years 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.

josevalim• 4 years 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• 4 years 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.

kamray233• 4 years 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.

AlchemistCamp2• 4 years 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-extr...

kamray233• 4 years 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• 4 years 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.

dgb232• 4 years 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/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• 4 years 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.

conradfr• 4 years ago

The "Complex multi-clause function" is debatable.

The refactored example forces the caller to use the specific function for each struct type which can be good or bad depending on the goal of the code.

Also due to syntax colouring I find the "bad code" actually more readable.

AlchemistCamp2• 4 years ago

It's remarkable, though maybe not very surprising, how much more subjective the design-related smells are than the low-level concerns smells.

trhway• 4 years ago

Kind of not surprising that "Large messages between processes" doesn't have a refactored solution :)

kamray233• 4 years ago

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.

ergl• 4 years ago

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.

pawelduda• 4 years ago

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

Lutger• 4 years ago

I just can't get over how much I dislike the word 'smell' in this context. I do know it is canon, unfortunately.

It makes me want to stop and do something else, something more pleasant. It evokes a nasty image of shovelling shit, where I want to think of what I am doing as polishing a raw gem.

Furthermore, to me it seems kind of disrectful to numerous code authors of such 'smells'. Maybe they didn't know better, maybe it used to be 'clean' but they were in a hurry to fix something. Maybe they made a different trade off. Labeling what they did as 'smelly' seems impolite and unnecessarily judgemental.

Objectively, it also narrows the perspective: something that smells is harmful, stale or poisonous. It affects disgust and moves us away from it. Food that smells has gone 'bad', which implies it is not good (anymore). So to fix a smell, is to move from bad to good, which is a very binary change. It's either good or bad.

But I think with code this not always the case, and even when some code is clearly better, seeing improvements as more appropriate or efficient tradeoff is almost always much more helpful.

Veen• 4 years ago

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.

whalesalad• 4 years ago

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?

notreallyserio• 4 years ago

Assuming the code works but is not ideal I'd just call it not ideal or inefficient or something similar.

dgb232• 4 years ago

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.

Calamitous• 4 years ago

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.

lucasvegi• 4 years ago

Exactly! Generally speaking, code smells are also context-sensitive structures. This means that a smell will not always represent pain in a specific project. It is just a possibility that must be analyzed in each context. It is a warning, not a judgment.

We are interested in knowing Elixir's community opinion about these code smells. Collaborations can be made via GitHub issues (https://github.com/lucasvegi/Elixir-Code-Smells/issues) or Pull Requests in our repo!

pabs3• 4 years ago

Are there tools for pointing out or fixing these sorts of issues with Elixir code?

jrmiii• 4 years ago

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

Catalog of Elixir-specific code smells

Related Stories