Close
Glad You're Ready. Let's Get Started!

Let us know how we can contact you.

Thank you!

We'll respond shortly.

LABS
Method Modeling: A Refactoring

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.

Comments
  1. theldoria says:

    Nice, but what if:
    – other modules are included that also define method_missing?
    – the class including AwesomeResource also defines method_missing?

    • Matthew Parker says:

      Great points! The answer is, of course, that bad things will happen. I may have to use `define_method` in the end.

  2. How about inheriting from HashWithIndifferentAccess and using Hash#fetch which would validate the presence of the key implicitly? Shouldn’t that work, too, and be even more concise because you wouldn’t need the custom #[] and #[]= methods?

    • Matthew Parker says:

      Nice Idea! At the moment, I’m not sure how much I want to commit to depending on active_support; it can make working with multiple versions of Rails trickier.

  3. pat brisbin says:

    So, there’s likely some domain knowledge that says this implementation can’t work, but I think it’s much simpler and passes the spec:

    https://gist.github.com/pbrisbin/5636088

    • Matthew Parker says:

      Thanks Pat!

      When you call attr_* methods on class, you define methods for all instances of that class, not just the current instance. You’d probably want to use `define_method` if you go that route. The main reason I didn’t use `define_method` is because once you save a resource, the server responds with the new authoritative attributes for that resource. If those attributes are different from what you sent, you have to undefine the methods. Not the end of the world, but it’s some manual work that I’ve decided to avoid for now.

      -Parker

  4. Rather than using method_missing why wouldn’t you define the setters / getters on initialize?

    • Matthew Parker says:

      Hi Quinn,

      Thanks for the question. The main reason I didn’t explicitly define methods is because after a save, the response that comes back from the server is the new authoritative version of that resource, and if the attributes are different from what you sent, you’ll have to go back and undefine the old attributes and define new attributes.

      Thanks!
      -Parker

  5. Edward Price says:

    Nice work.

    You mentioned early on that the AttributeAccessor contained the abstract method:

    def execute(*)
    end

    I have found that raising an error like the below helps in testing and development.

    def execute(*)
    raise “I am abstract”
    end

    Have you considered the performance pros/cons from using lambdas verses defining methods?

  6. James Gary says:

    `method_missing?` struck me as a round-about way of implementing this behavior. Using define_method` feels more explicitly about what you’re trying to do. Here’s my passing implementation: https://gist.github.com/jamesgary/5639529 (inspired by examples in http://apidock.com/ruby/Module/define_method)

  7. Diego Alonso says:

    I am with you, Pat. That’s what I would have implemented. It passes the specs, and it’s concise and clear.

  8. This looks a lot like my refactor of ostruct, astruct: http://github.com/krainboltgreene/astruct

  9. Thanks for the great refactoring write up! I like how you went through the steps, illustrating how a ‘fair-minded’ principle can save you from the “failure-by-strict-ideals” problem.

    I don’t like defining new classes with Struct.new( some_symbols), but I do occasionally use OpenStruct as the base for a model. Your AwesomeResource seems like a nice compromise between the two: your attributes are dynamic, but accessing non-existent ones raises an error.

    Then I started to poke around in irb until I came up with this:

    https://gist.github.com/russen/5645979

    The ‘if/else’ isn’t as OOP as your approach, but since the function is only 4 lines, it does pass Sandi Metz’s 4 rules :) (https://gist.github.com/henrik/4509394)

    I had fun trying out this different solution, and am completely interested in any +/- feedback you have!

Post a Comment

Your Information (Name required. Email address will not be displayed with comment.)

* Copy This Password *

* Type Or Paste Password Here *