10 ms·

The proof is always in the pudding. While there are good and reasonable times to introduce "interactors", this particular example is poor. The tests presented a

by dhh 13y ago

The proof is always in the pudding. While there are good and reasonable times to introduce "interactors", this particular example is poor. The tests presented are anemic, and the code is absolutely not any clearer by being extracted and wrapped. I would indeed have kept all this in the controller. The key point for an "interactor" extraction is imo when you have multiple models being created in symphony, like a Signup model. Or if you for some reason need to reuse the behavior. But if all your controllers look like this, with one "interactor" model per action, you're doing it wrong. Whatever floats your boat, though. If this is what you prefer, great. But please hold the "beginner's version" crap. Plenty of large apps are built with vanilla Rails. Basecamp is one.

tomblomfield 13y ago
I agree with you - Interactors are particularly useful when you're creating multiple models, and the example could have been better. But they're useful for dealing with side-effects of a particular operation too (sending multiple emails, notifying admins). Much better than ActiveRecord callbacks. If you put this logic in the controller, what happens when you want a separate API controller that does the same thing? Or some admin functionality elsewhere in the codebase? There's zero possibility of code re-use.
- malyk 13y ago
  I bet dhh is going to say something like "then refactor your controller into a service/interactor/etc., but not before". YAGNI, and all that.
- dhh 13y ago
  Coulda, woulda, damn shoulda. You're future coding with your "what ifs". Most controller actions are not reused. The time to extract for reuse is when you need to reuse. Not crystal balling about that you probably will be in the future. Because when the reuse case actually arrives, you might find that you need to reuse some, but not all of the action. If you're literally doing the exact same thing for both web and api, why are you using a separate API controller? Just use respond_to with formats. Second, yes, you shouldn't have side-effects like sending email in your models. But you don't need to! Just stick that logic in your controllers. That's what it's there for -- to render the views (of which emails is one of them -- see http://david.heinemeierhansson.com/2012/emails-are-views.html http://david.heinemeierhansson.com/2012/emails-are-views.htm...). The AR callbacks are wonderful for coordinating the domain model. Often times you'll want to create auxiliary objects when something else is created. That's what's it's there for. Or to otherwise keep the integrity of the domain model in place.
  - tomblomfield 13y ago
    Sure, I think we're saying the same thing here. This example is from a real-life codebase, and the interactor was extracted from 3 or 4 (bloated, repetitive) controllers. Perhaps this could have been made clearer in the post. I'm not arguing at all that every controller needs an Interactor, or all Rails codebases should start out with Interactors present. I am saying that Interactors are very useful concepts for re-using code, and that many medium and large Rails codebases could benefit from them.
    - dhh 13y ago
      Please do share the 3 or 4 controllers all sharing this logic. I'd be happy to play code pong with them.
  - dablweb 13y ago
    Regardless of reuse, controller actions can get awfully large if unchecked. What would you say is the maximum LOCs for a public controller method? Callbacks in Controllers and AR::Models tend to make things worse IMO for anything other than authentication; and for intricate actions interactors seem like the best defence. I have had the (mis)fortune to jump into a number of large Rails codebases and I can say with hand-on-heart that the only ones that made any sense off-the-bat were ones using a this-or-similar pattern. The community is embracing policies, interactors and decorators for a reason. Real developers are having real problems when Rails apps get a certain size, and there is no endorsed method on how to handle these problems. Surely a couple of asides on the Rails docs and some official endorsement could help point new developers in the direction of a possible solution? A solution the industry already seems to be taking; regardless of whether 37Signals deems it fit for their particular domain.
    - halostatue 13y ago
      > What would you say is the maximum LOCs for a public controller method? As small as it needs to be in order to get the job done, and as large as it must be to get it done clearly. Sometimes that's zero lines of code; sometimes it's 500. You can usually factor down a 500 line method, but it may be worth asking what the breadth cost is should the code really be single-use. If your metric is anything else, then you're playing Stupid Metrics Games, like that espoused by Code Climate.
- rgbrgb 13y ago
  >> If you put this logic in the controller, what happens when you want a separate API controller that does the same thing? You refactor. In my experience, anytime you're writing something for the sake of "possible code re-use", you're wasting time. Code should and does get refactored often. By adding levels of indirection from the outset, you add a barrier to refactoring and likely additional unnecessary code.
dhh 13y ago
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 https://gist.github.com/dhh/9333694
- dhh 13y ago
  Here's another version that doesn't even use private methods in the controller and uses a PORO for the email grouping: https://gist.github.com/dhh/9333991 https://gist.github.com/dhh/9333991
  - danso 13y ago
    Thanks for writing this. While I often look for ways to take external-interaction-responsibility away from Rails objects, the OP didn't really make sense to me, at least in terms of saving time and making things more logical. 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 Whereas your example, the controller would take the successful branch if the data model was saved, regardless of whether email delivery failed: if @grouper.save ConfirmedGrouperEmails.new(@grouper).deliver AssignBarForGrouper.enqueue(@grouper.id) redirect_to home_path else render :new end So we're not comparing apples to apples here. However, as a layperson, I'd have to agree with you: Why should the error be raised to the grouper-creating user, when it should be going to the part of the system that handles mailing? But maybe the actual details are more complicated than that...
    - dhh 13y ago
      Why would the delivery of the emails fail? Because your SMTP server is down? That's an exceptional state, handle it with exceptions -- not with conditions. Or maybe because the email addresses are invalid? Handle that when they are captured. It's way too late for that here.
    - 13y ago
rafekett 13y ago
the example is poor, butI think you're missing the core point here -- slow tests. I don't think this really improves readability or organization much -- but my real world experience with a giant slow rails test suite gives me 100% confidence that patterns like this are the only way to not have a completely intractable giant test suite.
- javiercr 13y ago
  Definitely agree. Would love to see DHH commenting on the slow tests problem.
halostatue 13y ago
I've only been involved in a couple of Rails projects (at least since the wedding list management app I wrote for my own wedding back in the v1 days), I'm in complete agreement with David. And honestly, Rails already has something that works beautifully for an “interactor” extraction. It's called ActiveModel. Add a bit of other code (include ActiveModel::ForbiddenAttributesProtection, ActiveModel::Validations::Callbacks, and ActiveModel::SerializerSupport; extend ActiveModel::Naming) and you've got an object that acts like an ActiveRecord object but orchestrates changes across multiple models in a clean, constructive way—without introducing an entirely different way of working with your code. I've done this at both places I've been doing Rails and it works at both cleaning up the models it orchestrates and keeping the business logic in the places where it belongs. I'm working actively to get rid of some of the Rails Fads that have come through and wreaked havoc on the current codebase (we've got Presenters and Commands and all sorts of other stuff that is just making the code harder to understand with little value). There's a million ways to skin this particular cat, and claiming that “adult” Rails needs this particular gem or that particular way of writing code says a lot more about the person claiming it than the people who aren't following that particular fad.
- dasil003 13y ago
  > And honestly, Rails already has something that works beautifully for an “interactor” extraction. It's called ActiveModel. The thing which Rails and DHH offer no guidance whatsoever is the separation of persistence from the domain model. The Active Record pattern is itself a conflation of those two things, so this is an opinionated choice, and I respect it as a sane default for a wide variety of applications. It works perfectly fine up to a certain scale but when the business logic reaches a certain complexity and the persistence logic reaches a certain complexity then it makes sense to separate them. A lot of proposed solutions might be overkill, but keep in mind that DHH and the company formerly known as 37signals specialize in minimalist software, so they combat this complexity from the UX down rather than conceiving architectures to support it. I agree with this philosophy insomuch as all else being equal, simpler is better, but the problem is that some applications are necessarily more complex than Basecamp, and sometimes we need more than what Rails provides out of the box. Convincing DHH of this is pointless because he doesn't have to deal with it and he has no incentive to understand anyone else's pain in this regard.
evilgeenius1 13y ago
Large - yes, but with an extremely simple domain model (and better for it).
markdodwell 13y ago
How long does the Basecamp test suit (excluding any integration tests) take to run?

by dhh 13y ago

tomblomfield 13y ago

I agree with you - Interactors are particularly useful when you're creating multiple models, and the example could have been better. But they're useful for dealing with side-effects of a particular operation too (sending multiple emails, notifying admins). Much better than ActiveRecord callbacks. If you put this logic in the controller, what happens when you want a separate API controller that does the same thing? Or some admin functionality elsewhere in the codebase? There's zero possibility of code re-use.

malyk 13y ago
I bet dhh is going to say something like "then refactor your controller into a service/interactor/etc., but not before". YAGNI, and all that.
dhh 13y ago
Coulda, woulda, damn shoulda. You're future coding with your "what ifs". Most controller actions are not reused. The time to extract for reuse is when you need to reuse. Not crystal balling about that you probably will be in the future. Because when the reuse case actually arrives, you might find that you need to reuse some, but not all of the action. If you're literally doing the exact same thing for both web and api, why are you using a separate API controller? Just use respond_to with formats. Second, yes, you shouldn't have side-effects like sending email in your models. But you don't need to! Just stick that logic in your controllers. That's what it's there for -- to render the views (of which emails is one of them -- see http://david.heinemeierhansson.com/2012/emails-are-views.html http://david.heinemeierhansson.com/2012/emails-are-views.htm...). The AR callbacks are wonderful for coordinating the domain model. Often times you'll want to create auxiliary objects when something else is created. That's what's it's there for. Or to otherwise keep the integrity of the domain model in place.
- tomblomfield 13y ago
  Sure, I think we're saying the same thing here. This example is from a real-life codebase, and the interactor was extracted from 3 or 4 (bloated, repetitive) controllers. Perhaps this could have been made clearer in the post. I'm not arguing at all that every controller needs an Interactor, or all Rails codebases should start out with Interactors present. I am saying that Interactors are very useful concepts for re-using code, and that many medium and large Rails codebases could benefit from them.
  - dhh 13y ago
    Please do share the 3 or 4 controllers all sharing this logic. I'd be happy to play code pong with them.
- dablweb 13y ago
  Regardless of reuse, controller actions can get awfully large if unchecked. What would you say is the maximum LOCs for a public controller method? Callbacks in Controllers and AR::Models tend to make things worse IMO for anything other than authentication; and for intricate actions interactors seem like the best defence. I have had the (mis)fortune to jump into a number of large Rails codebases and I can say with hand-on-heart that the only ones that made any sense off-the-bat were ones using a this-or-similar pattern. The community is embracing policies, interactors and decorators for a reason. Real developers are having real problems when Rails apps get a certain size, and there is no endorsed method on how to handle these problems. Surely a couple of asides on the Rails docs and some official endorsement could help point new developers in the direction of a possible solution? A solution the industry already seems to be taking; regardless of whether 37Signals deems it fit for their particular domain.
  - halostatue 13y ago
    > What would you say is the maximum LOCs for a public controller method? As small as it needs to be in order to get the job done, and as large as it must be to get it done clearly. Sometimes that's zero lines of code; sometimes it's 500. You can usually factor down a 500 line method, but it may be worth asking what the breadth cost is should the code really be single-use. If your metric is anything else, then you're playing Stupid Metrics Games, like that espoused by Code Climate.
rgbrgb 13y ago
>> If you put this logic in the controller, what happens when you want a separate API controller that does the same thing? You refactor. In my experience, anytime you're writing something for the sake of "possible code re-use", you're wasting time. Code should and does get refactored often. By adding levels of indirection from the outset, you add a barrier to refactoring and likely additional unnecessary code.

dhh 13y ago

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 https://gist.github.com/dhh/9333694

dhh 13y ago
Here's another version that doesn't even use private methods in the controller and uses a PORO for the email grouping: https://gist.github.com/dhh/9333991 https://gist.github.com/dhh/9333991
- danso 13y ago
  Thanks for writing this. While I often look for ways to take external-interaction-responsibility away from Rails objects, the OP didn't really make sense to me, at least in terms of saving time and making things more logical. 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 Whereas your example, the controller would take the successful branch if the data model was saved, regardless of whether email delivery failed: if @grouper.save ConfirmedGrouperEmails.new(@grouper).deliver AssignBarForGrouper.enqueue(@grouper.id) redirect_to home_path else render :new end So we're not comparing apples to apples here. However, as a layperson, I'd have to agree with you: Why should the error be raised to the grouper-creating user, when it should be going to the part of the system that handles mailing? But maybe the actual details are more complicated than that...
  - dhh 13y ago
    Why would the delivery of the emails fail? Because your SMTP server is down? That's an exceptional state, handle it with exceptions -- not with conditions. Or maybe because the email addresses are invalid? Handle that when they are captured. It's way too late for that here.
  - 13y ago

rafekett 13y ago

the example is poor, butI think you're missing the core point here -- slow tests. I don't think this really improves readability or organization much -- but my real world experience with a giant slow rails test suite gives me 100% confidence that patterns like this are the only way to not have a completely intractable giant test suite.

javiercr 13y ago
Definitely agree. Would love to see DHH commenting on the slow tests problem.

halostatue 13y ago

I've only been involved in a couple of Rails projects (at least since the wedding list management app I wrote for my own wedding back in the v1 days), I'm in complete agreement with David. And honestly, Rails already has something that works beautifully for an “interactor” extraction. It's called ActiveModel. Add a bit of other code (include ActiveModel::ForbiddenAttributesProtection, ActiveModel::Validations::Callbacks, and ActiveModel::SerializerSupport; extend ActiveModel::Naming) and you've got an object that acts like an ActiveRecord object but orchestrates changes across multiple models in a clean, constructive way—without introducing an entirely different way of working with your code. I've done this at both places I've been doing Rails and it works at both cleaning up the models it orchestrates and keeping the business logic in the places where it belongs. I'm working actively to get rid of some of the Rails Fads that have come through and wreaked havoc on the current codebase (we've got Presenters and Commands and all sorts of other stuff that is just making the code harder to understand with little value). There's a million ways to skin this particular cat, and claiming that “adult” Rails needs this particular gem or that particular way of writing code says a lot more about the person claiming it than the people who aren't following that particular fad.

dasil003 13y ago
> And honestly, Rails already has something that works beautifully for an “interactor” extraction. It's called ActiveModel. The thing which Rails and DHH offer no guidance whatsoever is the separation of persistence from the domain model. The Active Record pattern is itself a conflation of those two things, so this is an opinionated choice, and I respect it as a sane default for a wide variety of applications. It works perfectly fine up to a certain scale but when the business logic reaches a certain complexity and the persistence logic reaches a certain complexity then it makes sense to separate them. A lot of proposed solutions might be overkill, but keep in mind that DHH and the company formerly known as 37signals specialize in minimalist software, so they combat this complexity from the UX down rather than conceiving architectures to support it. I agree with this philosophy insomuch as all else being equal, simpler is better, but the problem is that some applications are necessarily more complex than Basecamp, and sometimes we need more than what Rails provides out of the box. Convincing DHH of this is pointless because he doesn't have to deal with it and he has no incentive to understand anyone else's pain in this regard.

evilgeenius1 13y ago

Large - yes, but with an extremely simple domain model (and better for it).

markdodwell 13y ago

How long does the Basecamp test suit (excluding any integration tests) take to run?