# 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

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

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


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

• Thank you for an excellent explanation. Re extracting the email validation to a separate class... where would that class go in terms of directory structure? At first though I think lib... great observation re the ENV variable. I did not give that much thought, but what you say makes perfect sense. Re the params, I'm only using message and emails keys. – Mohamad Mar 12 '14 at 18:40
• I would only add the memoize gem has a warning to use other gems. – Mohamad Mar 12 '14 at 18:44
• @Mohamad You are welcome. Thank you for the great code to review. Also, thanks for the tip about memoist. I've been using it a while and didn't realize it was deprecated. I've edited the answer to suggest memoist instead. In a rails project, lib is a good place for classes such as EmailAddress. – Wayne Conrad Mar 12 '14 at 19:00
• Notes on ENV, memoize, ... +1 Excellent answer – tokland Mar 12 '14 at 19:02