Creating a pure Ruby object (PORO) to email files in a Rails application

I wrote a service object that emails a list of files to a list of email addresses.

I am unsure about my emails method. It splits a string into an array of emails. It then loops over the array and rejects any elements that do not match the email regex. The return value is an array of valid emails stripped of whitespace.

Another thing I'm getting a bad feeling about is the caching of the bucket, @bucket ||= ..., and the url = get_urls temp variable in the send_email method.

class FileSharingService
  def initialize(files, params)
    @files = files
    @params = params
    @client = AWS::S3.new
  end

  def send_email
    urls = get_urls
    emails.each do |email|
      FileSharingMailer.email_files(email, params[:message], urls)
    end
  end

  def emails
    @params[:emails].split(',').reject do |email|
      email.strip!
      email !~ /\A[\w+\-.]+@[a-z\d\-.]+\.[a-z]+\z/i
    end
  end

  def get_urls
    @files.map do |file|
      bucket.objects[file.current_version.path].url_for(:get, expires: 1.week.from_now)
    end
  end

  def bucket
    @bucket ||= @client.buckets[ENV['S3_BUCKET']]
  end
end

Good job on

• Tightly focused methods that do what their name says

• Non-surprising formatting

• Testable design

This is pretty good code

Environment variable

There's a few things that bother me about this class's use of the S3_BUCKET environment variable.

• It makes unit test awkward, as the test must either set the environment variable and then restore it, or it must stub ENV#[], either of which is a minor pain.

• It hard-codes the environment variable name

My first thought was to have the class take the bucket name as an argument with a default:

def initialize(files, params, bucket_name = ENV['S3_BUCKET'])

But I think that is a half measure. Instead, let's just pass in the bucket:

def initialize(files, params, bucket)

bucket is an instance of this class, which does a lazy fetch of the AWS::S3 bucket:

require 'forwardable'
require 'memoist'

class Bucket

  extend Forwardable

  def initialize(bucket_name = ENV['S3_BUCKET'])
    @client = AWS::S3.new
    @bucket_name = bucket_name
  end

  def_delegator :bucket, :objects

  private

  def bucket
    @client.buckets[@bucket_name]
  end
  memoize :bucket

end

• The memoist gem Is a fancy version of "@bucket ||= ...".

• Forwardable#def_delegator delegates the #object method to the result of the #bucket method.

The use of the Bucket class has these advantages:

• FileSharingService:

• no longer has to know about the environment variable.

• does not have to concern itself with lazy-initialization and caching.

• can now be easily tested in isolation--just give it a test double for its bucket instance.

• Bucket can be easily tested in isolation.

Checking email addresses

I would consider taking the regular expression in this expression:

email !~ /\A[\w+\-.]+@[a-z\d\-.]+\.[a-z]+\z/i

And, at a minimum, give it a constant. The goal is to make the code more self-documenting:

WELL_FORMED_EMAIL_ADDRESS = /\A[\w+\-.]+@[a-z\d\-.]+\.[a-z]+\z/i
...
email !~ WELL_FORMED_EMAIL_ADDRESS

I might go one step farther and move the entire check into another class. Make it someone else's responsibility to know what's a good email address and what's not:

EmailAddress.well_formed?(email)

params

The only use the class makes of params is of @params[:emails]. If that is unlikely to change in the future, I would consider just passing in the email addresses, and not concerning this class with params at all.

def initialize(files, email_addresses, ...)

StackExchange Code Review Q#44173, answer score: 4