[dhh]

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

[rubiquity]

I think a lot of blog posts use trivial and contrived examples for the point of explaining the concept. There's not a lot of people out there that are good at explaining complicated refactorings. Katrina Owen comes to mind as one of the few that is good at explaining such refactorings.

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 ;)

[jlangenauer]

Isn't ConfirmedGrouperEmails here just an Interactor / Service Object anyway, but one that is derived from ActionMailer?

To my mind, this code is much clearer, which proves the OPs point.

[danso]

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]

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.

[Axsuul]

The failure of delivery of emails should be handled gracefully such as within a message queue that gets retried until it succeeds or dies.

[iamwil]

I can't seem to find it on Google. What's a PORO?

[cheshire137]

Plain Old Ruby Object, as opposed to an ActiveRecord model.

[dragonwriter]

Never seen the term used before, but by analogy to POJO and POCO it would have to be a "Plain Old Ruby Object".

[the_fury]

Plain Old Ruby Object