3
\$\begingroup\$

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
\$\endgroup\$
4
\$\begingroup\$

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

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.