I rewrote the code in this example to use the "Beginner's Version" of Rails (sigh). You judge which you like better: https://gist.github.com/dhh/9333694
Perhaps if someone made a post like this with starting code that was more complex it would be a better example, and also harder for you to counter it with a couple gists ;)
To my mind, this code is much clearer, which proves the OPs point.
However, there's a distinction that has to be made in your example and the OP's. In the OP, the failure of the Interactor, including the delivery of emails, would cause the controller to enter the "failure" branch:
if interactor.success?
redirect_to home_path
else
flash[:error] = interactor.message
render :new
end
if @grouper.save
ConfirmedGrouperEmails.new(@grouper).deliver
AssignBarForGrouper.enqueue(@grouper.id)
redirect_to home_path
else
render :new
end
Or maybe because the email addresses are invalid? Handle that when they are captured. It's way too late for that here.
> You obviously need to choose the patterns that fit the problem you’re trying to solve – it’s rarely one-size-fits-all, and some of these principles may be overkill in a very simple 15-minute blog application. Our objection is that Rails feels like it’s a Beginners’ Version, often suggesting that one size does fit all
This quote from the post (minus the "Beginners" jibe - sorry!) is my core objection. Your example works fine if you're working with a simple application. But the "Rails Way" feels like it starts to fall over when you get to medium or large codebases.
I feel like Rails could benefit hugely from a few core "Advanced" features that help you grow a small application into a larger, functioning business. Sure, you can add them yourself, but then you lose the advantage of shared standards and strong architectural conventions. Every new developer we hire has to learn what exactly we mean by "Interactors" or "Decorators" or "Policies".
Don't even get me started on the hand-wavy "loosely coupled".
If this is a poor example, pick a good example. I'll be happy to code ping pong you whatever example you choose.
One way to spend hundreds of thousands of LOC on an application is to stuff it with needless abstractions. That doesn't make it "advanced", and it's not Rails that's falling over, it's probably just some shitty code.
Maybe that's a clue that your chosen paradigms weren't that helpful. Or rather, that they convoluted the code base instead of clarified it. It's certainly a red herring that you need to teach your abstractions to new developers and that it's an endeavor to do so.
POROs are great, though. Our app/models is full of them. We even added app/services too. The trouble I have is with people who, like you, fall in love with the flawed notion that their application is such a special snowflake that it deserves "advanced techniques". Bullshit. There are good techniques and bad techniques. Size of the application is rarely a factor, except in ballooning the ego of the programmer.
Anyway, the invitation stands. Present any piece of real code and we can ping pong on it. I enjoy talking about specifics and improving real code. I detest "oh that was just a poor example, but the general principle is..." kind of debates. If you can't produce a good example, you don't have a general principle.
The main difference I see between the Rails philosophy and most of these "Use SRP and Interactors and things!" blog posts are that Rails is way more interested in using the correct tool for the job, and most of these blog posts are of the "use this for everything and every one!" variety.
I have one project right now that I decided I wanted a Command for one action. I'm accepting input from a webhook that I can perform asynchronously, it interacts with like 5 different models, and I wanted to beat up on the tests pretty thoroughly. So I did that.
Elsewhere I'm using concerns to make like 4 different models easily sortable.
And other places I'm using Module#concerning to keep models organized.
ONE TRUE WAY is overrated. I'd rather use whatever works given the context.
Cases have Tasks and Activities. Activities are always associated with a case, and may be associated with a task.
In the controller, when assigning a case, one would type: AssignCaseCommand.new(params).execute!
The command class implements the Command pattern (the pretty well established name for interactors) and is a Composite as well.
BTW - the same command that assigns the tasks of a case is also used to merely assign a task. And, if we're just recording an activity, we would just use the create activity command.
I think it would be insanity to try and capture the rules associated with re-assigning tasks in the controller. (I am pretty sure that you would agree.)
--Minor Edit for clarity--
I think the main problem with your command pattern approach is that you didn't implement the full command pattern. At least, I assume you didn't, you didn't actually show the controller. The command pattern has one extra class, the invoker. The invoker takes commands, and invokes them. A great example of this is the worker queue, it takes generic worker objects that have an 'execute' method.
It is never the class that creates the worker objects, that executes them. Yet that seems to be the way you're going to implement the controllers that use these commands.
What I think you've built instead is an adapter pattern. You have abstracted a class of problems in a way that they have a common interface, so that in your controllers you have a uniform way of invoking them.
This is where I think dhh sees the muddiness in your code. Why are you abstracting your interaction from the controller, which is supposed to be the exact thing that should be controlling the interaction.
The only reason for that would be that the same interaction is happening in different controllers in exactly the same way. And that's something that dhh claims does not happen in real applications, or at least he demands an example.
To address your comments:
While it is true that one of the great features (and frequent motivations) for using the Command pattern is the separation of the creation and invocation, it is not the only motivation. The two big payoffs that we are getting are:
- Separation of invocation from the knowledge of how it is carried out
- Composition of complex commands from smaller, simpler commands
There's no requirement that the invoker is never the same as the creator. It's just that most often it isn't, particularly (as you noted) in a worker queue system. That's really just a feature of the implementation that the command readily enables rather than a fundamental property of the pattern. For us, sometimes it is, sometimes it isn't; that's not the primary payoff that we're going for.
I can't support the characterization of this as an Adapter. This is fundamentally behavioral. I have a class that represents an action that I want to have happen. All of the places that want this action do not need to know what is involved, they only need to create the command and hand it off/invoke it.
FWIW - I have five different places in my software that create an AssignCaseCommand. - One controller
- Three commands that can create this as a sub command
- A utility job
Plus that simple concept is available to any custom implementation code that we write.
This is, of course, a primary advantage of having a static representation of the domain-level demands of your code—you can choose many ways and many environments to execute it within. It's a function I use in real code all the time.
It includes a lot of more context than your example, so I was guessing liberally at the missing pieces. But hopefully this is still easily digestible.
I don't know how to put this gently, but this concoction looks like a hot mess with no separation between modeling and controlling.
These commands have a ton of domain logic all tangled up with activity reporting, formatting, and parameter parsing. It looks like a big ball of mud to me :(
That is of course the most popular of all architectural patterns. :)
Edit: Curious why the downvote, I ask a legitimate question to the parent to provide a source that the command pattern is the most utilized architecture in the industry so please don't downvote without an explanation.
I disagree that re-assigning a case belongs in the case model. It is a complex interaction between several models. Extracting it to a command object allows all of the rules to be easily understood.
When I did something similar[1], I implemented it as a “virtual” model and applied validations to make sure that the PlanChange was properly constructed, and then performed the change in a transaction. I was able to test the hell out of it both with and without actual database interaction. But I was able to use it in three different locations and removed lots of special case code that had built up over two years of development.
You probably don't need Commands. You probably need something to orchestrate manipulating multiple ActiveRecord objects simultaneously in a transaction. It's still a model, but it may not be the model that you thought it was.
[1] Account from Plan [a] to Plan [b] that has a different quota, affecting Device assignments, possibly including moving some devices to Plan [b], some devices to Plan [z] (a default plan), and disabling some devices entirely because there's no quota available anymore.
Command, Virtual Model, Verb Model, Interaction. If I were working on your team I am sure we could agree that one of those would do for a name.
Unfortunately, it's easy to dismiss simple examples as not being worth the complexity and demand real examples which can be dismissed because they cannot be easily understood.
In this case, the Right Place (tm) to put these is in POROs / Interactors that can be tested in isolation and re-used across the code-base if necessary. This code should not live in controllers and certainly not in ActiveRecord models.
It sounds like you agree with that, since you're using them in your own codebase?
In that case, is there not an advantage to standardising the API for these POROs in Rails? Simple methods like fail!, success?, failure? and message
Another way of asking the question - why do so many Rails developers fail to realise that they can use these concepts? Why are so many Rails codebases packed full of enormous ActiveRecord models that are crippled with numerous before_save callbacks?
Sure, shitty coders write shitty code. But can't you lend them a hand?
Though I don't know if I agree that the actual sending of emails belongs in a PORO either and not a controller. It seems like determining whether or not emails need to be sent is the kind of logic I would put in a PORO and do the actual email sending from a controller method as in DHH's example.
Edit: Just saw DHH's second example, that's essentially how I would write it.
Key phrase: "if necessary".
It sounds like you're advocating for building abstractions before you actually have a reason to use them. Your reason seems to be "we may need to reuse them in the future" or, "one can see where we might move in a direction where we'd want want to re-use this code", or worse yet, "we're definitely going to build some functionality after this release when we'll want to re-use this stuff."
I've been there, really, I have. Build the abstraction when you actually need it, not when you think you're going to need it, or pretty close to needing it, etc. Because 98% of the time, YAGNI.
> In that case, is there not an advantage to standardising the API for these POROs in Rails?
Not a big enough that I've come across to warrant building domain-specific abstractions and special rules that, as @dhh mentioned, will serve only to raise the barrier to entry to your codebase.
If it sounds that way, I've over-simplified the examples in the blog post in the interests of clarity. I'll take more care in future! The specific interactor in question is used in 4 separate places.
From the post:
> You obviously need to choose the patterns that fit the problem you’re trying to solve – it’s rarely one-size-fits-all, and some of these principles may be overkill in a very simple 15-minute blog application.
It's not clear to me that adding more APIs and conventions that said shitty devs won't read about, understand, or even be aware of is actually useful.
If shitty devs are your problem, don't fix the dev tools - FIRE the tools pretending to be devs...
If you really want to standardize the interface, use the tools that already exist: ActiveModel and validations. Your “Interactors” are models; they just happen to be coordinating models that affect multiple ActiveRecord models simultaneously. (I did this for an account plan change system that affected billing and device assignment to a plan. It could easily affect a dozen or more records, all in a single transaction, but was itself easily testable.)
That's going in my quote book. Thank you DHH.
You are using app/services and POROs in your own app? So are we. But I guarantee they don't look like yours because there is no endorsed convention.
Why not? Because Rails is still clinging to such an outdated idea as MVC? Pure MVC at a large scale is bullshit.
"Size of the application is rarely a factor". I don't agree. Size of the application is always a factor. A big factor.
What I'd like to see is the community embrace conventions outside the fabled "MVC" paradise. Rails is "convention over configuration", and is content to hold your hand for your first thousand lines of code... but then what?
avoiding a new layer of abstraction because it can just about be done in vanilla rails isn't a solution, it's just throwing more lines of code at the problem.
recently i've found that everything i do at the frontend intermixed with the controller, models and helpers could be made simpler with two way databinding.
it's also simpler, if someone want's to see if it looks right you can plug in some fixtures and tweaking css can be done independantly instead of needing to write something in ruby to affect how something in html looks.
maybe for a project with a large codebase is to use lighter frameworks such as backbone rather than having to do full rewrites in emberjs or angularjs. and let the code rewrite itself, ie. just clean up the portions of code that were only there for presentation.
Also, I never said never. I said this particular example was a poor example and the general principle derived from it was equally poor. Garbage in, garbage out.