I'm writing code for a worker receiving message from a queue.

It should

  • send an email
  • save the fact that this email has been sent
  • acknowledge the job
  • log some info.

I wrote the following code, but I find it ugly!

Do you have some advice on how to refactor it ?

class MailWorker
  def initialize(logger, db, smtp)
      @logger = logger
      @db = db
      @smtp = smtp

  def handle(message)
    start = Time.now.utc.to_f
    data = JSON.parse(message.body, symbolize_names: true)

    smtp_resp = send(data)
    save_as_sent(data[:id], smtp_resp['request_id'])

    @logger.info("Mail send #{data[:id]} start:#{start} spent:#{(Time.now.utc.to_f - start)}")

  rescue Aws::SQS::Errors::ServiceError, Smtp::Error => e
    @logger.error("Mail error for mail #{data[:id]} #{e.backtrace.join("\n")}")


  def send(data)
    @smtp.send(data[:to], data[:subject], data[:body])

  def save_as_sent(id, request_id)
    @db.prepare('UPDATE Mail set request_id = ? WHERE id = ?')
    @db.execute(request_id, id)
  • \$\begingroup\$ Without seeing more of this class, it's hard to know what can be done to make it cleaner. To be honest, it doesn't look that bad as it stands. \$\endgroup\$ – Greg Burghardt Jan 6 '17 at 12:35
  • \$\begingroup\$ Please show the rest of your class. You are potentially missing a MessageHandler object which this logic could be extracted into. It looks like the method has way too many responsibilities. But, as Greg said, it's difficult to know without seeing the rest of the class. What I do know is passing arguments into all your methods is a code-smell - handle(message), send(data), save_as_sent(..etc..). \$\endgroup\$ – Damien Roche Jan 6 '17 at 20:45
  • \$\begingroup\$ Thank you for your comments, I just added the rest of my class. \$\endgroup\$ – runewby Jan 7 '17 at 14:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.