Simpler SRP

explanation.md

There's ways to do cleaner SRP that allow moving email delivery out of a model and controller and still be flexible for emailing different things based on type of user, etc.; it's merely a matter of how you solve the problem. simple.rb is the simplest thing that'll do basically the same exact thing that the original author intended - separate object, fast to test, blah blah blah. Introducing DI to override delivery is super simple and can be a lambda/Proc or method; WithDeliveryEmail doesn't give a shit, as long as it has a call method accepting the user to deliver the email to.

Is the solution of moving email delivery outside the callback of the user sensible? Can other developers figure out what the side effects of saving a user from the controller immediately? Does creating a user always ensure that an email is delivered? If the system does what it needs to do and all those can be answered with yes, I think it's a reasonable solution.

simple.rb

class UsersController < ApplicationController
  def create
    @user = User.new(params[:user])
    WithDeliveryEmail.new(@user).save
    respond_with @user
  end
end

class WithDeliveryEmail < Struct.new(:user)
  def save
    user.save && UserMailer.deliver_welcome_email(user)
  end
end

with_admin_or_anything_else.rb

class UsersController < ApplicationController
  def create
    @user = User.new(params[:user])

    WithDeliveryEmail.new(@user).save
    # or
    WithDeliveryEmail.new(@user, ->(user) { AdminMailer.deliver_welcome_email(user) }).save
    # or
    admin_delivery = AdminMailer.method(:deliver_welcome_email)
    WithDeliveryEmail.new(@user, admin_delivery).save

    respond_with @user
  end
end

class WithDeliveryEmail < Struct.new(:user, :delivery_method)
  def save
    user.save && delivery_method.call(user)
  end

  def delivery_method
    @delivery_method || UserMailer.method(:deliver_welcome_email)
  end
end

I like the simplistic interface of Procs, which is ultimately the appeal here, IMO. The containment of the call to #deliver_welcome_email is nice as well. This would be an interesting technique if this code grew to multiple actions after a save, potentially supporting threading or background workers:

class UsersController < ApplicationController
  def create
    @user = User.new(params[:user])

    WithSubsequentProcessing.new(@user, ->(user) { AdminMailer.deliver_welcome_email(user) },
                                        ->(user) { Twitter.post_join_to_stream(user)       },
                                        ->(user) { Facebook.post_join_to_wall(user)        }).save

    respond_with @user
  end
end

class WithSubsequentProcessing
  def initialize(user, *queue)
    @user, @queue = user, queue
  end

  def save
    @user.save && @queue.each{ |process| process.call(@user) } # Is there a shorter way to construct this each block?
  end
end

@joshuaclayton

I think your code does too much just to avoid a conditional. I hate conditionals as much as the next guy, but sometimes they can't be avoided. Nice code though!

Regarding the article, the author ended up creating a AdminUsersController, which would have been enough to avoid the conditional, which also revealed the root problem: The UsersController was not following SRP (creating two "types" of users).