---
title: ActiveRecord, Caching, and the Single Responsibility Principle
teaser:
tags: web,rails,good code
author: Josh Clayton
published_on: 2011-08-19
---

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!