I was working on a messaging system earlier this week and noticed a pretty tight coupling between two classes.
class Message < ActiveRecord::Base
belongs_to :author, :class_name => 'User'
belongs_to :conversation, :touch => true
after_create :author_reads_conversation
after_save :update_answered
after_save :update_last_student_posted_at
after_save :update_last_post_at
after_save :update_participants
after_save :update_messages_count
# ...
private
def update_answered
if conversation.kind == Conversation::QUESTION || conversation.kind == Conversation::ASSIGNMENT
if author.instructor?
conversation.update_attribute(:answered, true)
else
conversation.update_attribute(:answered, false)
end
end
end
def update_last_post_at
conversation.update_attribute(:last_post_at, self.created_at)
end
def update_last_student_posted_at
if author.student? || conversation.last_student_posted_at.nil?
conversation.update_attribute(:last_student_posted_at, self.created_at)
end
end
def update_participants
participants = {}
conversation.contributors.each{|user| participants[user.id] = user.profile.full_name }
conversation.update_attribute(:participants, participants.to_json.to_s)
end
def update_messages_count
conversation.update_attribute(:messages_count, conversation.messages.count)
end
def author_reads_conversation
conversation.read_by!(author)
end
end
The first thing I noticed was that all of these callbacks are actually
interacting with the conversation, not the message. A message shouldn’t be
responsible for updating a handful of attributes on another model! The second
thing I noticed was that Message#update_messages_count could actually be
handled by a counter_cache.
My gut reaction was to move all of this onto Conversation and call that one
method in a callback on Message. Why? I want to call one method that handles
all the data caching from a conversation’s standpoint.
Based on the conditionals, it looked like I could get away with passing the author and be done with it. After pondering how to test that (it would’ve been pretty nasty), I decided to extract it into a separate class. With a seperate class, I can then stub methods on conversation (because stubbing the system under test is evil) as well as the message.
class ConversationCacher
def initialize(conversation, message)
@conversation = conversation
@message = message
end
def run
update_last_posted_at
update_last_student_posted_at if @message.author.student?
update_answered if @conversation.answerable?
cache_participants_as_json
save_conversation
author_reads_conversation
end
private
def update_last_posted_at
@conversation.last_post_at = @message.created_at
end
def update_last_student_posted_at
@conversation.last_student_posted_at = @message.created_at
end
def cache_participants_as_json
@conversation.participants = participants_as_json.to_s
end
def update_answered
@conversation.answered = @message.author.instructor?
end
def author_reads_conversation
@conversation.read_by!(@message.author)
end
def save_conversation
@conversation.save(:validate => false)
end
def participants_as_json
@conversation.contributors.inject({}) {|result, user| result[user.id] = user.profile.full_name; result }.to_json
end
end
This class deals with all attribute caching on the Conversation. It’s
now incredibly easy to test:
describe ConversationCacher, "#run" do
let(:conversation) { create(:conversation) }
let(:message) { create(:message) }
let(:student) { create(:student) }
let(:instructor) { create(:instructor) }
subject { ConversationCacher.new(conversation, message) }
it "updates conversation#last_post_at" do
subject.run
conversation.last_post_at.should == message.created_at
end
context "when the message author is a student" do
before { message.author = student }
it "updates conversation#last_student_posted_at" do
subject.run
conversation.last_student_posted_at.should == message.created_at
end
end
context "when the message author is an instructor" do
before { message.author = instructor }
it "does not update conversation#last_student_posted_at" do
subject.run
conversation.last_student_posted_at.should be_nil
end
end
# ...
end
Here’s what Message looks like now:
class Message < ActiveRecord::Base
cattr_writer :cacher
def self.cacher
@@cacher || ConversationCacher
end
belongs_to :author, :class_name => 'User'
belongs_to :conversation, :touch => true, :counter_cache => true
has_many :attachments, :dependent => :destroy
after_create :cache_data_on_conversation
private
def cache_data_on_conversation
self.class.cacher.new(conversation, self).run
end
end
Notice that I created a Message.cacher class method. If unset, it’ll return my
ConversationCacher. Now I won’t have to stub ConversationCacher.new to
make sure that my cacher works properly.
describe Message, "after save" do
let(:author) { Factory(:user) }
let(:conversation) { Factory(:conversation) }
let(:cacher_class) { stub("cacher", :new => cacher) }
let(:cacher) { stub("cacher instance", :run => true) }
subject { Factory.build(:message, :author => author, :conversation => conversation) }
before { Message.cacher = cacher_class }
after { Message.cacher = nil }
it "triggers the conversation data be cached for query optimizations" do
subject.save
cacher_class.should have_received(:new).with(conversation, subject)
cacher.should have_received(:run)
end
it "caches only on create" do
2.times { subject.save }
cacher_class.should have_received(:new).with(conversation, subject)
cacher.should have_received(:run).once
end
end
describe Message, ".cacher" do
after { Message.cacher = nil }
context "when overridden" do
let(:cacher) { stub("my awesome cacher") }
before { Message.cacher = cacher }
it { Message.cacher.should == cacher }
end
context "when not set" do
before { Message.cacher = nil }
it { Message.cacher.should == ConversationCacher }
end
end
One-line methods, small contexts in our tests, and an easy understanding of
what’s happening after a message is created. I’d call this a successful
refactor. The Message now doesn’t care whatsoever about what data is cached,
since that logic is on ConversationCacher. If more data needs to be cached
on the conversation when a message is created, it’s immediately obvious where
that logic is handled.
In my experience, after_* callbacks on Rails models seem to have some of the
most tightly-coupled code, especially when it comes to models with
associations. Do yourself a favor and decouple some code!
