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.

Hey Sasa, thank you for the thoughtful post, it’s quite fantastic to be able to discuss these matters with someone of your expertise, everyone learns so much which to me is the whole point of all of this.

I’m considering changing some wording on the article because I wasn’t clear in the sense that I do not suggest starting with such a rigid structure from the beginning. I chose to slowly present my proposed convention with easy to understand examples while keeping the post short and dense as I feel that is a more effective way of communicating to a broader range of people.

As @baldwindavid mentioned, the article is aimed to provide structure to a larger codebase. I completely agree that small abstractions need to have a small code footprint and up until you start having multiple sub-systems, keeping everything under one context can be much better as you pointed out.

I’ll have to test your suggestion of putting changesets in the service module as well, I’ve had situations where some business logic leaked into changesets and questioned if they should really be in Schemas, but given that is how it’s presented on the Phoenix guides, I chose to keep that convention the way it is – again, that is why I think talking about this is so valuable.

Edit: added this section to the introduction

It’s important to note that the conventions laid out here are focused on optimizing larger codebases, so if you have a small project, following the patterns set by the Phoenix generators is completely fine and will make you more productive. It’s always best to start with simple abstractions and refactor as the project evolves.

Yeah, both Phoenix and Ecto docs propose keeping changeset functions in schemas. This is one of the things where I disagree with “the blessed” way" (some other examples being overuse of app config and organization of web files by controller/view/template role).

When it comes to business logic leaking in changesets, I’d argue that this will be the case anyway, no matter where you put these functions. The thing is that Ecto is a somewhat “dirty” concept because it mixes domain modeling and data storage concerns. So, for example, changeset functions typically contain some business validations. Now, to be clear, I think this is actually a good thing, b/c we can start simple without all the ceremony of transferring the data to library/framework independent data structures and back. I also believe that this simple approach can scale far with respect to complexity. And finally, I feel that it should be easy enough on the beginners, without limiting the options for more experienced programmers.

But either way, the consequence of typical Ecto usage is this mixing of persistence and domain concerns, as well as the fact that the application view of the world maps exactly to the relational model, which is not always perfect.

Regardless, I think that intertwining of changeset operations with some amount of business logic is fine, as long as it’s not overdone. However, keep in mind that this will cause the context to grow more rapidly, and when it becomes too large, I suggest splitting by paying attention to cohesion, i.e. extracting things which naturally belong together, and separating things which are completely independent. When I’m doing this, I first perform a casual scan through the module to bootstrap my brain and grow some refactoring ideas, i.e. pick some group of functions which could be extracted to another module. Then I cut-paste one function, and rely on compiler warnings/errors to move the rest of the associated code. Once I get the project to compile & tests to pass, I reflect about the new shape of the code. If I’m not happy I might revert it and try something else, or I might try further splits, or maybe move some things back. Sometimes I find that the new state is not really better than the previous one, and then I just postpone refactoring until another time.

It’s not a straightforward process, and it requires some critical thinking, but I feel it leads to a much better separation of concerns compared to mechanical division based on secondary properties (like e.g. putting changesets here, and queries there).

There will be more complex domains where mixing Ecto with business logic is going to be noisy, but I feel that in such cases it’s better to explore pure domain modeling, i.e. transferring data from Ecto schemas to plain data structures (which might be organized in a significantly different way than the relational model), and doing all business logic on this pure model. In such approach Ecto will still be useful for data transfer, but it then becomes more like a supporting infrastructure, and should probably be moved out of the context, or burried deeper in some internal persistence subcontext.