-
-
Save gma/3128475 to your computer and use it in GitHub Desktop.
Separation of concerns sketch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
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!