There are parts of this article that I enjoyed, but then there are some which I don’t agree with. In general, I find the design of this example overly elaborate, and some suggestions overly generalizing. For example:
… how we can structure our business logic with a simple convention focused on developer productivity. For every database table, I suggest having 3 supporting modules: a Schema, a Query and a Service.
I have to say that I don’t find this proposal very productive focused. Granted, it adds a strong structure to the code, which can be followed mechanically, but I feel that such structure is pretty shallow, i.e. it is focused on splitting the code by non-essential properties. As a result, things which would better fit together from a readers perspective are kept separate. For example, let’s consider the create function:
def create(attrs) do
Hero.changeset(%Hero{}, attrs)
|> Repo.insert()
end
Here we immediately delegate to another module, and now I have to open a second file to see what’s happening before the insert. It’s also worht noting that Rpg.create_hero
already delegated to Heroes
, so basically one line of code in, and I already have three files open, keeping the stacktrace in my mind, and I’m no wiser about what goes on. I have to say I don’t find this particularly helpful or productive 
Another indication that there is something amis with this design is in the fact that changeset
is only meant to be used by the Heroes
service. Yet we return heroes struct to the web layer, so it is free to invoke changeset/1
even though it makes no sense.
Yet another clue emerges if we try to type this function:
@type create(attrs) :: Ecto.Changeset.t()
This is a very concrete abstraction, and yet the signature of its API function is completely generic. What’s in this changeset? I have no clue, I have to read the code to understand. Basically the delegate doesn’t bring anything useful, but forces me to jump back and forth in the code.
In this particular case, I’d address this by moving the changeset function to the service layer, and given its size, I’d inline it directly into Heroes.create/1
. Going further, I feel that the whole Heroes
service is an overkill for such a small program. I’d move the code of Heroes.create/1
to the Rpg context, and now we end up with:
defmodule Rpg do
def create_hero(attrs) do
hero
|> cast(attrs, [:level, :is_enabled, :gold])
|> Repo.insert()
end
end
which I believe is much easier to read and maintain 
Going further, I’d also inline queries into the query functions, and move those to Rpg
as well. With that, we lose two modules, and consolidate the code of all operations in a single place, making it easier for the reader to understand what each operation does.
Now, granted, as the code grows, the Rpg
module will become bloated. However, instead of upfront design based on guesswork and some bureaucratic rules, I prefer to let the context grow a bit, and then perform the split based on the actual code which at the time exactly corresponds to the requirements. The gain is that at that point we have a deeper understanding of the world (because we’ve spent some time working on it), and we have a better understanding of requirements (because we implemented them), and so we have some concrete data from which we can see distinct groups of code.
A nice example could be the items concern. In the article, the code is small enough that I’d place this logic in Rpg
. But over time, I can imagine some split might be needed. I prefer to wait until we have enough of real code implementing actual requirements to see how that split will happen. Many times, I’ve witnessed that if I wait, things turn out to be much different than I originally guessed, because the requirements change, and because our understanding of the domain deepens.
Sometimes it’s worth splitting upfront. A nice example of this is the accounts logic. We can be pretty certain that this logic will have some complexity (registration, authentication, password resets/changes, profiles, etc), and that this will be mostly independent from our main logic. Hence, immediately placing account operation into the Accounts
context makes sense, though I personally wouldn’t mind if the initial implementation is stashed in Rpg.
I hope you won’t take this criticism too hard. I’ve seen some real damage that can arise from applying these principles mechanically. A team I consulted, did that, and they ended up with a huge amount of micro-modules in a relatively small code base. There was a strong sense of structure, and yet the code was incredibly confusing (not just to me, the original developers also didn’t like it), because of the large amount of code delegation and inter-module dependencies. Basically, it was hard to tell the forrest from the trees in such a codebase, and to me this is not particularly better than stashing all code in a single bloated module.
The good modularity is IMO obtained by keeping together things which naturally belong together, while separating the things which don’t. This is not done to satisfy some academic principles, but to simplify the lives of the people that have to plow through the code on a daily basis. Ideally, a single module contains everything I need to know, while pushing aside everything I don’t. This will never be completely possible, as there’s always some work cutting through concerns, but the code can be organized to be close to that ideal in most typical cases.