Types of Coupling

Those of you coming from a Google search are about to be disappointed: this is a post about types of coupling in programming.

Coupling refers to the degree to which components in your program rely on each other. You should generally seek to minimize this property, though you’ll see it’s impossible to eliminate entirely.

Here are a few types of coupling, ordered by severity (first is worst):

Pathological Coupling

Your class reaches inside another class and reads (or, perish the thought, changes) its instance variables.

You are literally pathological and deserve the pain this will cause you.

class NuclearLaunchController
  def initialize(launch_codes)
    @launch_codes = launch_codes
  end
end

class ExtremelyBadIdea
  def initialize(nuclear_launch_controller)
    @launch_controller = nuclear_launch_controller
  end

  def do_bad_things
    # This is poison
    @launch_controller.instance_variable_set(:@launch_codes, 'password')
  end
end

As mentioned by Mike Burns in the comments, monkey-patching falls into this category. Beware!

Global Coupling

You have two classes that both rely on some shared global data, maybe a Singleton or a class variable.

When many test files all depend on global factory definitions, a change to any one can ripple through the system.

# spec/factories.rb
FactoryGirl.define do
  factory :user do
    # Changes here are global and can affect many test files.
  end
end

# spec/model/user_spec.rb
before do
  # This refers to global data.
  @user = build_stubbed(:user)
end

# spec/model/order_spec.rb
before do
  # So does this.
  @user = build_stubbed(:user)
end

Note that this is probably an example where the cure (duplicating the logic for creating test objects in every spec) is worse than the disease.

Control Coupling

You pass in a flag that tells a method what to do.

Remember save(false) in ActiveRecord? That boolean argument caused control coupling. Remember how we all had to change our code when it became save(validate: false)? If we’d been calling save and save_without_validation instead, Rails Core could have refactored that method more times than the router and we’d never have known. Also, notice that passing validate: false into save does not reduce the coupling, it’s just disguised better.

Control couples are smelly because the calling method has intimate knowledge of how the receiver implements the method being called. You’re determining what an object should do from outside it. Good OOP lets objects decide what to do based on their own state.

def save(should_run_validations=true)
  # When you see a parameter in a conditional, that's control coupling.
  # The fact that this method has an if in it has leaked out into client code.
  # Changes can now require changes in these clients.
  if should_run_validations
    run_validations
    persist
  else
    persist
  end
end

# One possible fix: define two methods and let the clients 
# choose which to call. Now we can refactor either without 
# affecting clients.
def save
  run_validations
  persist
end

def save_without_validations
  persist
end

Data Coupling

You call a method and pass it a parameter that doesn’t affect its control flow.

This is still coupling, but we’re starting to reach the kind that isn’t so bad. Sometimes you need parameters! If you wanted to remove all coupling you wouldn’t be able to pass data between objects at all.

class ScreenPrinter
  # This method is coupled to its parameter, because a change to that argument 
  # can cause breakage (if we undefined to_s, for example).
  def print(text)
    output_to_screen(text.to_s)
  end
end

Message Coupling

You call a method on an object and send no parameters.

You’re coupled to the name of the message, but not any hint of its implementation. This is the loosest type of coupling and should be your goal. Notice that this makes methods that take no arguments better than methods that take one (and so on).

# No reliance on anything outside this object. Feels good, man.
class ScreenPrinter
  def print_to_screen
    output_to_screen(@text)
  end
end

Keep an eye out for the nasty types of coupling in your code, and see if you can can’t refactor it into something further down the ladder.

(I cribbed this list of coupling types from Wikipedia’s article, paraphrased and added examples. The original article is worth reading.)

ActiveRecord, Caching, and the Single Responsibility Principle

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!