Separation of concerns sketch

soc-to-the-max.rb

class TasksController < ApplicationController
  def complete
    # add responder as listener, or could subscribe etc...
    # task could be the actual task, or pass through the ID
    task.add_subscriber(TaskCompletedResponse.new(self))
    task.add_subscriber(TaskEmail.new)
    task.add_subscriber(TaskIndex)
    task.complete_by(person)
  end

  class TaskCompletedResponse < SimpleDelegator
    def task_completed(task, person)
      # render success JSON etc...
    end

    def task_completion_failed(task, person)
      # render failure JSON etc...
    end
  end 
end


## PORO's << This is a largely irrelevant discrimination; think about
#            responsibilities instead

class Person
  attr_accessor :name
  attr_accessor :email
end

class Assignee
  attr_accessor :person
  attr_accessor :completed_at

  def completed?
    self.completed_at.present?
  end
end

class Task < Struct.new(:description, :assignees)
  attr_reader :completed_at

  def completed?
    self.completed_at? || assignees.all? { |a| a.completed? }
  end

  def complete_by(person)
    assign_completed_at(person)
    TaskRepository.complete_task(self, person, completed_at)
    publish(:task_completed, self, person)
  rescue SomePersistenceError, Task::PermissionDenied, Task::AlreadyCompleted
    publish(:task_completion_failed, self, person)
  end

  # This should definitely raise, rather than return true/false. God
  # knows why the Rails community have eschewed exception handling, but
  # this `if @foo.save` pattern is a balls up. Tell-don't-ask style
  # would be to instruct the object to do something; only if it blows up
  # might you want to do something about it. The idiom is called EAFP
  # (easier to ask forgiveness than permission). Rubyists do LBYL (look
  # before you leap). The fools...
  #
  # And I'd try and avoid all this returning early stuff; it's bug
  # heaven. I've nuked it.
  def assign_completed_at(person, time = Time.now)
    if ! completable_by?(person)
      raise Task::PermissionDenied
    end

    assignee = assignees.detect { |a| a.person == person }
    if assignee.completed?
      raise Task::AlreadyCompleted
    else
      assignee.completed = time
      self.completed_at = time
    end
  end
end

class TaskRepository
  include Repository::Mongo

  collection_name 'tasks'

  def self.find(id)
    data = collection.find_one( ... )
    assignees = data["assignees"].map { |a_data| Assignee.new(a_data) }
    Task.new(data["description"], assignees)
  end

  def self.complete_task(task, assignee, time)
    collection.update({_id: task.id, 'assignees._id' => assignee.id}, 
                      {:$set => {"assignees.$.completed_at": time}})
  end
end

class TaskIndex
  include Search

  index_name 'tasks'

  # ...
  
  def self.task_completed(task, person)
    index_task(task)
  end

  def self.index_task(task) 
    index.store(task.id, json_for(task))
  end

  # ...
end

class TaskEmail
  def self.perform(task_id, person_id)
    task = TaskRepository.find(task_id)
    task.assignees.reject { |a| a.person_id == person_id }.each do |assignee|
      TaskMailer.new(task, assignee).deliver
    end
  end

  def send(task, person)
    Resque.enqueue(self.class, task.id, person.id)
  end

  def task_completed(task, person)
    send(task, person)
  end
end

Nice. Actual app uses exceptions as you mention, completely agree with rationale there!

Need to think some more about class distinction RE Assignee / Person. In the actual app I have the following:

• Account
• User (someone with a login)
• Person (member of the account, may or may not be a User - IE may not yet have been invited yet)
• Project
• Project::Member
• Task
• Task::Assignee (decorated Person who is assigned the task, could just be a Person but we also have completion time etc...)

All the above are MongoMapper documents mirroring the DB structure, except for ProjectMember which is purely domain object as in the db membership is stored elsewhere.

I like that you've separated out emailing / indexing as subscribers to the task action, that makes sense now. Was originally hesitant (hence the TaskCompleter class) as it felt like that wasn't the controllers decision to make, but actually looks fine.

Would be nice if the domain objects (Task etc...) didn't know about persistence either, but I feel that starts to get rather complex and goes counter to maintainability in this case.

Gists need inline comments!