gistfile1.md

Which do you prefer and why?

Task contains enqueue_at an optional attribute that defines when (hour and minute) the job should be enqueued. If the attribute is not present, the current time should be used.

# tasks (id INT, enqueue_at DATETIME, action VARCHAR(255))
class Task < ActiveRecord::Base
end

Each day we need to enqueue all of the tasks. Below are two examples and I'm looking for opinions on which is better and why.

Example 1

Task.all.each do |task|
  if task.enqueue_at
    enqueue_at = Time.now.change(:hour => task.enqueue_at.hour, :min => task.enqueue_at.min)
    Q.enqueue_at(enqueue_at, Task, task.id)
  else
    Q.enqueue(Task, task.id)
  end
end

Example 2

class Task
  def enqueue(queue = Q)
    if enqueue_at
      enqueue_at = Time.now.change(:hour => enqueue_at.hour, :min => enqueue_at.min)
      queue.enqueue_at(enqueue_at, Task, id)
    else
      queue.enqueue(Task, id)
    end
  end
end

Task.all.each do |task|
  task.enqueue
end

I was actually working on code very similar to this today, which is what inspired me to ask for feedback. Today I chose to write Example 2, but on another day I may have written Example 1.

Example 1 may be able to be tested without loading ActiveRecord, but enqueuing a task requires reaching into each Task object to see if enqueue_at is set and to get the id.

Example 2 requires ActiveRecord to be loaded in order to write tests against it. However, it only requires knowledge of Q and it's interface, enqueue and enqueue_at.

Example 1 would be preferable if you expected to have different enqueing strategies, I think, because it would be external to the Task class and could be more easily swapped out with a different strategy.

But, if you don't expect to need different enqueing strategies I think Example 2 is the clear winner. Example 2 is better encapsulated (you don't have to expose the "enqueue_at" field outside of the class). It also takes the Q as a parameter which should make it more testable since you could supply a stubbed implementation of Q during test time as opposed to depending on a concrete instance as in Example 1. I also like the separation of the selection of Tasks and the enqueueing of tasks (which could be done in Example 1 by extracting a helper function as well I guess)

FWIW

Hey @geofflane thanks for the feedback! What do you think about something like this replacing the loop in Example 1?

module EnqueuesTasks
  module_function

  def enqueue(tasks = Task.all, queue = Q, job = Task)
    tasks.each do |task|
      if task.enqueue_at
        enqueue_at = Time.now.change(:hour => task.enqueue_at.hour, :min => task.enqueue_at.min)
        queue.enqueue_at(enqueue_at, job, task.id)
      else
        queue.enqueue(job, task.id)
      end
    end
  end
end

Hi Michael —

I like your modified example 1, because it keeps the enqueue method out of Task, which as an ActiveRecord object, is likely to already be somewhat bloated.

Not trying to hijack the point of your post, but I'm curious why you choose to make it a module rather than a class?

I'd probably write it this way...

class EnqueueTasks
  def initialize(tasks = Task.all, queue = Q)
    @tasks ||= tasks
    @queue ||= queue
  end

  def enqueue_all
    @tasks.each { |task| enqueue(task) }
  end

  def enqueue(task)
      @queue.enqueue_at(enqueue_at(task), task.class, task.id)
  end

  def enqueue_at(task)
    if task.enqueue_at
      Time.now.change(:hour => task.enqueue_at.hour, :min => task.enqueue_at.min)
    else
      Time.now
    end
  end
end

@mguterl, the third one almost feels a bit "over engineered" to me (in the absence of requirements, context, etc). The selection of the tasks to enqueue definitely seems like a good thing to keep external from the main algorithm. Likewise passing the Q in seems like a good idea for testability at least. Example 2 and Example 3 both share those attributes, but example 3 also has the negative of the "asking" for the enqueue_at like Example 1 as opposed to keeping it encapsulated.

You could also think polymorphism for the type of Task to enqueue by modifying Example 2 to use self.class instead of explicitly Task:

class Task
  def enqueue(queue = Q)
    if enqueue_at
      enqueue_at = Time.now.change(:hour => enqueue_at.hour, :min => enqueue_at.min)
      queue.enqueue_at(enqueue_at, self.class, id)
    else
      queue.enqueue(elf.class, id)
    end
  end
end

Then if you had subclasses of Task they could still be enqueued without overriding the enqueue method that would give you the same effect as job = Task from Example 3.