Good job on
- Tightly focused methods that do what their name says
- Non-surprising formatting
- Testable design
This is pretty good code
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:
def initialize(bucket_name = ENV['S3_BUCKET'])
@client = AWS::S3.new
@bucket_name = bucket_name
def_delegator :bucket, :objects
The use of the Bucket class has these advantages:
- 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:
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, ...)