0
\$\begingroup\$

I seem to find my self writing a lot of classes like the below where there seems to be some kind of send method that just calls a bunch of other methods. Is this bad practice? Is there a more object oriented way to do this?

The below class uses User, UserDetail (preference object), and a custom Mailchimp api client. It basically goes through creates a campaign, adds the html body to that campaign and then sends it.

# Generate and send mailers
class Mail
  def initialize(user_id)
    @user = User.find(user_id)
    @detail = @user.details
    @client = @user.mailchimp
    @campaign_id
  end

  def send
    create_campaign
    create_html_content
    send_campaign
  end

  private

  def create_campaign
    params = @detail.create_campaign_params
    @campaign_id = @client.create_campaign(params)
  end

  def create_html_content
    params = @detail.create_content_params
    @client.create_campaign_content(@campaign_id, params)
  end

  def send_campaign
    @client.send_campaign(@campaign_id)
  end
end
\$\endgroup\$
  • \$\begingroup\$ What is @campaign doing as the return value from the initializer? \$\endgroup\$ – David Aldridge Jul 29 '17 at 11:42
  • \$\begingroup\$ I think you meant@campaign_id, it doesn't really do anything as a return value. I put it there for clarity of all the global variables available to this class. \$\endgroup\$ – Cameron Barker Jul 30 '17 at 22:19
1
\$\begingroup\$

I find it weird that you wrote this as a module rather than as a class.

How is this code going to be called? Mail.new(user_id).send, I assume? It doesn't seem to make much sense to call #send twice on the same object. So, really, there isn't any point to make an object at all. You could just as well define a function.

def send_mail_to_user(user_id)
  user = User.find(user_id)
  client = user.mailchimp
  campaign_id = client.create_campaign(user.detail.create_campaign_params)
  client.create_campaign_content(campaign_id, user.detail.create_content_params)
  client.send_campaign(campaign_id)
end
\$\endgroup\$
  • \$\begingroup\$ Oh I made a mistake it should have a been a class, I've updated the code. Thanks for the feedback. I did imagine it being sent by Mail.new(user_id).send so that would make sense. In the example you gave, would you see that function fitting more into a module? \$\endgroup\$ – Cameron Barker Jul 29 '17 at 0:13
0
\$\begingroup\$

I would definitely go with a separate class or with a factory that solely represents the concern this campaign encloses.

On top of, I would assume (though not necessary), you could separate the mailchimp related implementation detail under a different class, which simply knows how to deal with it. Your actual code not suppose to know that low level detail.

However, it could be easily reachable through the campaign builder instance. What if you decide something else instead of Mailchimp ?, Just replace the instance with SomethingCampaignBuilder instance.

Based on those thoughts, I would rewrite to the following code -

class MailchimpCampaignBuilder
  attr_reader :user

  def initialize(user)
    @user = user
  end

  def create_campaign(attrs)
    mailchimp_client.create_campaign(attrs)
  end

  def update_campaign_content(campaign_id, attrs)
    mailchimp_client
     .create_campaign_content(campaign_id, attrs)
  end

  def deliver(campaign_id)
    mailchimp_client.send_campaign(campaign_id)
  end

  def mailchimp_client
    @mailchimp_client ||= user.mailchimp
  end
end

class UserCampaignBuilder

  CreateCampaignError = Class.new(StandardError)
  UpdateCampaignError = Class.new(StandardError)

  attr_reader :user, :campaign_id, :campaign_builder

  def self.deliver(user, campaign_builder)
    builder = self.new(user, campaign_builder).build
    builder.deliver
  end  

  def initialize(user, campaign_builder)
    @user             = user
    @campaign_builder = campaign_builder
  end

  def build
    initiate_new_campaign
      .create_new_campaign
      .update_campaign_content
  end

  def deliver
    campaign_builder.deliver(campaign_id)
  end

  private

  def initiate_new_campaign
    @campaign_attributes          = user.detail.create_campaign_params
    @campaign_content_attributes  = user.detail.create_content_params    

    self
  end

  def create_new_campaign
    @campaign_id = campaign_builder.create_campaign(@campaign_attributes)

    raise CreateCampaignError unless @campaign_id.present?

    self
  end

  def update_campaign_content
    updated = campaign_builder
      .update_campaign_content(campaign_id, @campaign_content_attributes)

    raise UpdateCampaignError unless updated

    self
  end
end

# Usags:
user              = User.find(134)
campaign_builder  = MailchimpCampaignBuilder.new(user)

UserCampaignBuilder.deliver(user, campaign_builder)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.