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

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

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

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

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

1 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

  def_delegator :bucket, :objects


  def bucket
  memoize :bucket


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

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:



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, ...)
  • \$\begingroup\$ 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. \$\endgroup\$
    – Mohamad
    Mar 12, 2014 at 18:40
  • \$\begingroup\$ I would only add the memoize gem has a warning to use other gems. \$\endgroup\$
    – Mohamad
    Mar 12, 2014 at 18:44
  • \$\begingroup\$ @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. \$\endgroup\$ Mar 12, 2014 at 19:00
  • \$\begingroup\$ Notes on ENV, memoize, ... +1 Excellent answer \$\endgroup\$
    – tokland
    Mar 12, 2014 at 19:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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