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

There's a thousand ways to skin this cat. I don't find any proposed implementation interesting, honestly. Primarily because this problem is far too contrived.

Regarding this particular implementation, I'm not a huge fan though I find some things I like.

What I like:

• There's less code than the original proposal.
• Short circuiting on user.save instead of checking validity.
• The intention is expressed in the controller. I personally like the procedural read controllers have. It's the one place where procedural code feels right and it's nice to sometimes have a break from the overhead of OO.

What I don't like:

The cognitive overhead involved with understanding your intentions here is beyond that of the original proposal. To read this code, I have to perform the following conscious steps (hope you don't mind honest thoughts):

1. Ok, the email delivery class is saving the user? Oh no, it's some form of decoration.
2. Nope, not decoration. The WithDeliveryEmail#save method is just coincidencidentally named the same as User#save. ERRR. #save_and_send might better indicate it's purpose but this could quickly get out-of-hand.
3. simple.rb, I get it.
4. Procs, why? To accommodate methods other than #deliver_welcome_email? A polymorphic antipattern. What's wrong with just passing the class? FUUU
5. A #delivery_method to manage default values? We have default method params for that.
6. Where's my whiskey?

While even the original blog code was over-engineered for this contrived example, you've added another layer of abstraction, Procs, and you end up managing them all over the place.

What I dislike about passing around Procs is the mix of OO and functional paradigms. Functional works great in JS but feels second class in Ruby. OOP is much better suited for Ruby than it is in JS. I like to leverage the powerful aspects of the particular language I'm using and I tend not to mix and match unless absolutely necessary. I don't feel this contrived example warrants the cognitive overhead of mixing paradigms.

Aside

You'll likely use user.save! instead and catch the exception in the controller to display/redirect the correct content. Which is why I have gripes with the original authors claim that we can be avoiding Java's "wrong way":

try {
  someCode();
}
catch (Exception ex)
  ex.printStackTrace();
}

My Preference

FWIW, for this contrived example, I would much prefer to read this in a controller:

class UsersController < ApplicationController
  def create
    if @user = User.create(params[:user])
      UserMailer.deliver_welcome_email(@user)
    end

    respond_with @user
  end
end

The example I gave using procs/methods was to demonstrate that you could use whatever responded to call to be passed in as a delivery strategy to handle overriding the mailer. Passing in an object responding to deliver_welcome_email would've obviously been fine too but it seemed like the author wanted a bunch of flexibility.

I agree, though, contrived example - I just wanted to demonstrate that there were much easier ways to skin a cat.

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