Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Smell suggestion: import in busines logic code #30

Open
pdgonzalez872 opened this issue Oct 17, 2023 · 3 comments
Open

Smell suggestion: import in busines logic code #30

pdgonzalez872 opened this issue Oct 17, 2023 · 3 comments
Labels
suggestion of smell Elixir-specific smells suggested by the community

Comments

@pdgonzalez872
Copy link

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:

  • use import with libraries only
  • don't use with business logic code for "code organization" reasons

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
Copy link
Owner

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

@lucasvegi lucasvegi added the suggestion of smell Elixir-specific smells suggested by the community label Oct 21, 2023
@pdgonzalez872
Copy link
Author

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
Copy link
Owner

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion of smell Elixir-specific smells suggested by the community
Projects
None yet
Development

No branches or pull requests

2 participants