While working on AwesomeResource, I needed to implement functionality that would make the following test pass:

  it "creates readers and writers for any attributes passed in during initialization" do
    article_class = Class.new do
      include AwesomeResource
    end

    article = article_class.new("title" => "Fun")
    article.title.should == "Fun"

    article.title = 'Fun Times!'
    article.title.should == 'Fun Times!'

    expect { article.unknown_attribute }.to raise_exception
  end  

Simple enough. If you initialize an instance of a class that includes AwesomeResource, then you get attribute readers and writers for any hash keys passed in during initialization.

My first attempt at implementation looked something like this:

module AwesomeResource
  attr_reader :awesome_attributes

  def initialize(attributes={})
    @awesome_attributes = AwesomeResource::Attributes.new attributes
  end

  def method_missing(method_name, *args)
    if method_name["="]
      if awesome_attributes.has_key?(method_name[0...-1])
        awesome_attributes[method_name[0…-1]] = args.first
      else
        super
      end
    else
      if awesome_attributes.has_key?(method_name)
        awesome_attributes[method_name]
      else
        super
      end
    end
  end
end

Yuk. All those nested if/else’s didn’t sit well with me. Let me try to clean that up:

  def method_missing(method_name, *args)
    if method_name["="] && awesome_attributes.has_key?(method_name[0...-1])
      awesome_attributes[method_name[0...-1]] = args.first
    elsif awesome_attributes.has_key?(method_name)
      awesome_attributes[method_name]
    else
      super
    end
  end

The best I can say for this code is that it’s more compact. But is it any more readable? Hardly. In fact it’s worse.

Let’s take another look at the first implementation. The nested if/else blocks look awfully similar. Perhaps there’s a polymorphic model lurking there? But what are we modeling? If we were to wrap a class around those blocks, we’d have to be modeling a method. What would that look like? Let’s extract some classes:

  class AttributeWriter
    attr_reader :attributes

    def initialize(attributes)
      @attributes = attributes
    end

    def call(attribute_name, attribute_value)
      raise "Unknown Attribute" unless attributes.has_key?(attribute_name)
      attributes[attribute_name] = attribute_value
    end
  end

  class AttributeReader
    attr_reader :attributes

    def initialize(attributes)
      @attributes = attributes
    end

    def call(attribute_name)
      raise "Unknown Attribute" unless attributes.has_key?(attribute_name)
      attributes[attribute_name]
    end
  end

  def method_missing(method_name, *args)
    if method_name["="]
      AttributeWriter.new(awesome_attributes).call(method_name[0...-1], *args)
    else
      AttributeReader.new(awesome_attributes).call(method_name)
    end
  end

OK, I at least feel like I’m trying to write OO code at this point. There’s a bit of duplication between the AttributeReader and AttributeWriter classes. We could clean that up with template methods:

  class AttributeAccessor
    attr_reader :attributes, :attribute_name

    def initialize(attributes, attribute_name)
      @attribute_name = attribute_name
      @attributes = attributes
    end

    def call(*args)
      raise "Unknown Attribute" unless attributes.has_key?(attribute_name)
      execute(*args)
    end

    def execute(*)
    end
  end

  class AttributeWriter < AttributeAccessor
    def attribute_name
      super[0...-1]
    end
    
    def execute(attribute_value)
      attributes[attribute_name] = attribute_value
    end
  end

  class AttributeReader < AttributeAccessor
    def execute
      attributes[attribute_name]
    end
  end

  def method_missing(method_name, *args)
    if method_name["="]
      AttributeWriter.new(awesome_attributes, method_name).call(*args)
    else
      AttributeReader.new(awesome_attributes, method_name).call
    end
  end

Notice that we've moved the responsibility of stripping off the "=" off the method_name from method_missing down into AttributeWriter.

Hmmm... The base class is pretty abstract. How easy it this code to understand now?

Also, the body of the method missing now looks a lot like a factory method. While in Rome...

  class AttributeAccessor
    attr_reader :attributes, :attribute_name

    def self.from_method_name(attributes, method_name)
      if method_name["="]
        AttributeWriter.new(attributes, method_name)
      else
        AttributeReader.new(attributes, method_name)
      end
    end

    def initialize(attributes, attribute_name)
      @attribute_name = attribute_name
      @attributes = attributes
    end

    def call(*args)
      raise "Unknown Attribute" unless attributes.has_key?(attribute_name)
      execute(*args)
    end

    def execute(*)
    end
  end

  class AttributeWriter < AttributeAccessor
    def attribute_name
      super[0...-1]
    end

    def execute(attribute_value)
      attributes[attribute_name] = attribute_value
    end
  end

  class AttributeReader < AttributeAccessor
    def execute
      attributes[attribute_name]
    end
  end

  def method_missing(method_name, *args)
    AttributeAccessor.from_method_name(awesome_attributes, method_name).call(*args)
  end

I'm now starting to question this code. Is there a good trade off here between indirection and maintainability? I don't think the number of accessors are likely to grow (accessors have consisted solely of getters and setters since the dawn of OO). Between all of the refactorings, I think the first (with its minor duplication) was the easiest to understand.

Yet something still doesn't feel right. Let's look back at the very first code snippet. Notice the `AwesomeResource::Attributes.new` in the `initialization` method? Those are our bag of attributes (they're basically a hash with indifferent access). We keep passing it around to all of our Attribute* classes… perhaps some of this code should have lived there in the first place?

module AwesomeResource
  attr_reader :awesome_attributes

  def initialize(attributes={})
    @awesome_attributes = AwesomeResource::Attributes.new attributes
  end

  def method_missing(method_name, *args)
    awesome_attributes.accessor_for_method_name(method_name).call(*args)
  end
end

module AwesomeResource
  class Attributes < SimpleDelegator
	#...

    def accessor_for_method_name(method_name)
      if method_name["="]
        ->(attribute_value) { self[method_name[0...-1]] = attribute_value }
      else
        -> { self[method_name] }
      end
    end

    def [](key)
      validate_key_exists(key)
      attributes[standardized_key(key)]
    end

    def []=(key, value)
      validate_key_exists(key)
      attributes[standardized_key(key)] = value
    end

    private
    def validate_key_exists(key)
      raise "Unknown attribute" unless has_key? key
    end
    
    #...
  end
end

This feels right. We're now modeling methods with lambdas (Ruby's built in method objects). We've eliminated a tangle of nested if/else blocks without introducing too much indirection. We've maintained high-cohesion within the AwesomeResource::Attributes class despite the new methods.

Lunchtime Drawtime happens Fridays at lunch in the NYC office, where a handful of folks get together and perform some acts of creation. Sometime we have prompts; today’s was bodies of water.

Here’s what we came up with. Enjoy!