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.