lucasvegi / Elixir-Code-Smells

Catalog of Elixir-specific code smells
https://doi.org/10.1007/s10664-023-10343-6
MIT License
1.47k stars 48 forks source link

Smell suggestion: `import` in busines logic code #30

Open pdgonzalez872 opened 1 year ago

pdgonzalez872 commented 1 year ago

Hi @lucasvegi!

Thanks for the repo and your work on Elixir Smells!

I think there is a somewhat of a rule of don't use imports in business logic code (please point me to where this is documented/discussed if I missed this already) in a lot of the codebases that I've been around. For good reason too, (in my opinion, this is subjective though) because it adds a lot of indirection/mental load to reading code. I prefer knowing exactly where a function is defined if possible (Foo.bar) rather than trying to find where bar comes from and getting sad it's not in the file I'm working on.

There is an interesting thread in EF that discusses it and this opinion echos what I have seen in practice: https://elixirforum.com/t/anyone-wish-we-could-import-the-basics-without-conflicts/50567/2

The gist of it is:

I'm not sure how the mechanism to suggest smells really is, so I'm just creating this issue to get the ball rolling.

Thanks again!

lucasvegi commented 1 year ago

Hi @pdgonzalez872 !

I believe you are correct. The excessive use of imports can indeed hinder the 'traceability' of the origin of certain functions when we are reading and trying to understand code. Additionally, imports can lead to naming conflicts with locally defined functions in the importing module.

Nevertheless, when a module A calls many functions from a module B, it might be a good idea for A to import B, making the code less bulky by not having to explicitly refer to B multiple times.

I'm mentioning just a few positive and negative points because I can see various trade-offs between using or not using imports in certain situations. I appreciate your contribution very much, and I will take it into consideration!

Feel free to continue our conversation about this potential smell here. :)

pdgonzalez872 commented 1 year ago

For sure tradeoffs.

I just don't think I've seen the real benefit of not using the module before the function call. It's one of those things that can help you WRITE code but not READ it. For reading, the best I've seen has been to be very explicit and using the module before the function is one of those small things that do help.

I'm not sure I buy the "bulky code" argument, but I may just be getting old. No little surprises, no gotchas.

It's been one of the small details I've had to show when pairing with folks new to the language. "No, this here is a function from X" (In this case, it was an Ecto one). "So, I can just use put_in here? Yep.". The put_in one was nice, we looked at Kernel (docs and source) and it was clear to mention that all of Kernel's functions are imported and available. A couple of days ago I was reviewing some code and saw an unnecessary use of import, that's why I wanted to create this issue here and check if I'm going nuts.

Thanks @lucasvegi ❤️

lucasvegi commented 1 year ago

@pdgonzalez872 You're not going crazy, that's for sure! 😄

Leave it to me. I'll try to develop the idea of this potential smell

Adzz commented 1 month ago

If helpful I wrote this a long time ago: https://gist.github.com/Adzz/a37ce7fe0228e385ff5ad49ebccdeab4

The basic principle being something like:

Full module name > aliased module > require > explicit import > import everything