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

Question

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

Answer 1

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

• Mohamad: 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: I would only add the memoize gem has a warning to use other gems.
• Wayne Conrad: @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.
• tokland: Notes on ENV, memoize, ... +1 Excellent answer