sheharyarn
Need code review
I’m writing (my first) hex package to introduce “Ecto shortcuts” in an elixir/phoenix app. I was getting really tired of doing this all the time:
PhoenixApp.Repo.all(User)
# and
PhoenixApp.Repo.get(User, 2)
It should allow you to simply do:
User.all
User.get(2)
# etc...
I have a few concerns with this project, especially if I might be going against Elixir patterns. I would really appreciate if I could get some feedback on the code before I publish it.
See the Code here
Currently, my main concerns are:
- Is the naming scheme correct? (i.e. is
Ecto.Shortcutsa good name?) - Is doing this a good idea?
- Are there any anti-patterns in the code?
- Am I using macros correctly?
- What should I keep in mind while publishing this as a hex package?
Marked As Solved
Qqwy
Welcome! Great that you’re creating a Hex package.
Let’s try to answer your questions:
-
Your naming scheme is correct. While any names for packages are of course allowed, the advised convention is to indeed use
Foo.Barif you wrote a packageBarthat extends the more general packageFoo. In cases there exists both aBarand aFooand you wrote some kind of compatibility-layer between them, the order doesn’t matter (so you could go for e.g. alphabetical order there). So the nameEcto.Shortcutsis great!
Arguably. Some people will agree, others won’t. I myself would say that it is nice to be able to shorten Repo-based things inside your Controllers, (and saying that you basically should never talk to the Repo anywhere else). If Ecto.Shortcuts is the nicest way of doing so, is again very much a matter of taste.
It does mean that your ‘model’ is again directly connected with the repository, which is something that Ecto itself explicitly didn’t do. If you’re for instance dealing with multiple repositories that you want to switch between (a common example of this is Sharding), this is a problem.
The most important thing that might be considered an ‘antipattern’ is that your code is still missing documentation. In Elixir, documentation is a first-class member of the language, and most people try to add documentation to their functions.
I don’t see any other obvious anti-patterns at first glance. A really helpful tool to increase the quality of your code is Credo, which tries to point out some anti-patterns and inconsistencies in your code.
-
The way you are using
__using__/1is correct
.
Besides the fact that documentation is important, be sure to use Semantic Versioning for your package, because this means that when other people list a certain version as dependency requirement, their code will not break if you decide update your package.
Also Liked
josevalim
The issue with the approach above is that you are coupling your schema with one particular database and the schema is designed to represent any data source, which may not even be a database in the first place. We explore some of those things here: Ecto’s insert_all and schemaless queries « Plataformatec Blog
My concern with using something such as shortcuts is that people will forget those basic tenets schemas and repositories were built on. So I would rather define modules such as MyApp.Accounts.get_user(id), which is the direction Phoenix is taking, without a need to tie repositories directly into schemas.
Misha
Those sound eerily familiar… is it from the README of my own project you seem to have ripped off and claimed as your own?
Anyways, I don’t want to get petty on this forum, but I had to call out what seems like plagiarism to me. Ironically, this thread was a good run down of the issues to address in own project since it was reviewing a proxy of it.
Before I knew about this, I was already in the process of adding complete test coverage and updating to support the new Phoenix idiom that Jose pointed out. Though I am slightly annoyed, it is good validation that I was working on the right upgrades.
Sorry to introduce myself to this community in such a negative light, but I hope to have a lot of positive things to contribute in the future.
sheharyarn
Awesome! Thank you for your feedback!
Once I get all of the functionality working, I’ll start working on the documentation. One more question, how would the tests look like for it? I have a very limited experience with TDD, and I’m not exactly sure how would I go about writing tests for macros.
Qqwy
You’re very welcome!
Test Driven Development means that you first write tests and then fill in your code so they pass, which is not what you’re doing here. (Which is, of course, also a fine approach. 
Each way has its own advantages and drawbacks.)
The thing you probably want to test here, and also the way you can test the macro, is by simply creating a few modules that include the using Ecto.Shortcuts line. You could then test if the commands are properly passed on to the actual Repo when you call them on your test modules.
Another possible test is to test what happens if using Ecto.Shortcuts is added to a module that actually does not contain an Ecto schema.
If you need inspiration, feel free to pop open any of Elixir’s core libraries, such as for instance Ecto itself to see how they write tests there. Most of them are very understandable and of (at least in my opinion) a high quality.
And, especially when your modules start getting more complex, it might be worthwhile to check out José Valim’s post on using Mocks during testing.
Popular in Questions
Other popular topics
Latest on Elixir Forum
Sponsor Spotlight
Enabling companies to succeed by building software people love.
Categories:
Sub Categories:
Forums
Our Sponsors
Build Elixir applications with speed and confidence.
Supporting innovation across the BEAM ecosystem.
We build reliable cloud platforms for business-critical systems.
Catch errors, track performance, monitor hosts and more.
Practical resources that improve the lives of professional developers.
Producing high quality Elixir screencasts since 2017.
Enabling companies to succeed by building software people love.
Courses that'll move you from confusion to "Aha, now I get it!"