Has Many has_many: A Refactoring Story

Noel Rappin, Friday, November 14, 2008 @ 12:23 pm

Image is a Pieceless Puzzle, from Ceaco

Lately I've been working on a revision of one of my first Pathfinder projects, an internal agile management tool. Along with adding and rearranging some features, I'm also taking the opportunity to modernize some of the Rails 1.2 era code up to new Rails 2.1 and 2.2 features.

Note 1: I don't always (or often) recommend a wholesale update of working code when updating features. But this was a large enough feature change that cleaning up the code is worth the effort.

Note 2: When the time comes for you to perform a major refactoring like this remember that your tests have all the institutional memory about how the application should work. What I actually did was create a new blank project, and copy test files one by one. For each file, I commented all the tests, then uncomment them one at a time, rearranging the test as needed to match the new data model. Sometimes I take the new code directly from the previous version, sometimes I rewrite using a newer or better idiom. This gives me the benefits of test-driven design in my big refactor, while still preserving all the functional specification in my tests. (This seems to work better on models than controller/views, I'll probably have a fuller report next week).

There was one particular design decision in the original code that had always bothered me, but I could never think of a better solution.

In this system, the basic unit is the story. A story can belong to several other different entities in the system -- one or more users, an application, a specific iteration, and so on. Each of those entities, obviously has a has_many relationship with stories, and each one has a mostly-common set of functionality needed from that relationship -- how many stories have been done, how many are left, how many stories were done last week, and so on.

My original design placed all those common methods in a StoryGroup module. Each class that used stories had a declaration like this:

class Application < ActiveRecord::Base
  include StoryGroup
  has_many :stories
  ## and so on
end

Within the story group, there were a lot of methods that performed calculations and called back to class methods of Story, in part, that might look like this...

Module StoryGroup
  def remaining_expected_hours
    Story.expected_hour_total(remaining_stories)
  end 

  def remaining_tasks
    stories.select{ |each| !each.complete? }
  end
end

Class Story
  def self.expected_hour_total(story_list)
    if story_list.blank? then return 0 end
    story_list.to_a.sum(&:expected_hours)
  end
end

Honestly, I can't quite recreate the rationale for having the expected_hour_total method as a class method of Story. I think it's because there was a usage within the Story class before there was a need for it among the StoryGroup classes.

I have two main problems with the structure of the old code. First, having to include the module and declare the has_many relationship always struck me as kind of dodgy. Second, calling back to a class method of Story that takes a list of stories as an argument is really ugly looking, and the class methods kind of gunk up the Story class with a lot of calculations that don't necessarily belong there.

Everything works in the old version, but I was pretty sure there was a cleaner way. In the new version I solve both problems (I think). The first problem is solved by taking a page from your basic acts_as plugin. The second problem is solved with named scopes. Named scopes are particularly helpful here, since they apply to both Story as a class and to any has_many :stories associations. As a result, all the common story functionality is easier and clearer to describe.

The new StoryGroup functionality is a module, now placed in the config/initializers directory. This seems a bit verbose, but most of it is the standard boilerplate for an ActiveRecord plugin -- the main functionality will be in the InstanceMethods submodule

module ActiveRecord
  module HasManyStories

    def self.included(base)
      base.extend(ClassMethods)
    end

    module ClassMethods
      def has_many_stories
        has_many :stories
        include ActiveRecord::HasManyStories::InstanceMethods
      end
    end

    module InstanceMethods
      def remaining_expected_hours
        stories.uncompleted.sum(:expected_hours)
      end
    end

  end
end
ActiveRecord::Base.send(:include, ActiveRecord::HasManyStories)

On initialization, the has_many_stories method is added to ActiveRecord::Base. Any ActiveRecord class that calls the method, then gets all the instance methods included within it (I've left out common class methods, because they are not needed for this example)

Adding the story relationship and the common methods is now down to a single line within the class:

class Application < ActiveRecord::Base
  has_many_stories
end

In the new module, the remaining_expected_hours method has changed, now calling the named scope stories.uncompleted rather than a class method. The scope definition itself is straightforward.

class Stories
  named_scope :uncompleted,
      :conditions => {:state => ["new", "in_progress", "blocked"]}
end

As an added bonus, using the scope and appending sum to the scope now does the calculation in the database, which should be faster than the earlier version, which was calculating the sum in Ruby.

I like this cleanup. The individual methods within StoryGroup are now significantly simpler. The named scopes they depend on are also simpler and less distracting in the class then the previous class methods. Because the scopes are compossible, it's much easier to add calculations to the various slices of stories that the system needs.

UPDATE: to clarify something pointed out in comments. Yes, it would be possible to put all the instance methods in the HasManyStories module as class methods of Story. They would be polymorphic with associations in the same way that named scopes are. I admit I didn't really consider that option in my refactor. I'm conflicted about it -- it's clearly simpler, since it avoids the tricksy module load, but a lot of the methods in HasManyStories seem odd if placed as class methods of Story, that's part of why I wanted to move them in the first place...

What do you think? Have you had a similar problem? Is there a better way to solve it?

agile ajax

Comments: 5 so far

• Leave a comment
• Comment feed for this post
• TrackBack URL

1. 

   Great refactoring story

   I love refactoring my old code and seeing progress I made.

   Only one thing I would change - I would rather not place this kind of modules in initializers. If one of the gems fails initializers won’t be run and you will end up with en error.

   Cheers

   Comment by Piotr Sarnacki, Saturday, November 15, 2008 @ 6:07 am

2. 

   The general problem is what to do when you have a collection of something and you want to have operations on the collection. Rails says: “put the collection operations on the class” and makes the class methods available (with the correct scope) through the access methods.

   So, create Story.remaining_uncompleted_hours and call it through the stories collection.

   Here’s how I would do it (with a simplified implementation):

   Here’s the test. I’m creating a couple of users to demonstrate that the scoping is working

   class UserTest true, :hours => 2
   joe.stories.create :complete => false, :hours => 2
   joe.stories.create :complete => false, :hours => 2

   frank = User.create
   frank.stories.create :complete => true, :hours => 2
   frank.stories.create :complete => false, :hours => 2
   frank.stories.create :complete => false, :hours => 2

   ################################################
   # this is how calls through the attribute look
   assert_equal 4, joe.stories.remaining_uncompleted_hours
   end
   end

   Story has a class method named remaining_uncompleted_hours
   class Story {:complete => false}

   ################################################
   # here’s where I define the class method.
   def Story.remaining_uncompleted_hours
   uncompleted.sum(:hours)
   end
   end

   The simplified schema
   ActiveRecord::Schema.define(:version => 20081121185349) do

   create_table “stories”, :force => true do |t|
   t.integer “user_id”
   t.boolean “complete”
   t.datetime “created_at”
   t.datetime “updated_at”
   t.integer “hours”
   end

   create_table “users”, :force => true do |t|
   t.datetime “created_at”
   t.datetime “updated_at”
   end

   end

   A super-simple user model
   class User < ActiveRecord::Base
   has_many :stories
   end

   Comment by Vishu, Friday, November 21, 2008 @ 2:47 pm

3. 

   Yikes - I’d love a do-over on the formatting there! Feel free to contact me. I can just send you the code or try again with a little more formatting instruction.

   Comment by Vishu, Friday, November 21, 2008 @ 2:52 pm

4. 

   I agree that it feels weird to put the collection operations on the story class. It’s just that that seems to be the intended rails way.

   I also might prefer to put the collection operations on a pluralized class. Something like:

   module Stories
   def remaining_uncompleted_hours
   uncompleted.sum(:hours)
   end
   end

   I played around with some implementation options and the best I could come up with was:

   class Story {:complete => false}
   end

   With a plugin :

   module ActsAsCollectible
   def self.included(base) # :nodoc:
   base.extend ClassMethods
   end

   module ClassMethods
   def acts_as_collectible
   # determine the plural using pluralize
   # todo: take the name of the module as option
   self.extend self.name.pluralize.constantize
   end
   end
   end

   Comment by Vishu, Monday, November 24, 2008 @ 3:56 pm

5. 

   That says

   class Story < ActiveRecord::Base
   acts_as_collectible

   named_scope :uncompleted, :conditions => {:complete => false}
   end

   I think < got interpreted as a tag.

   Comment by Vishu, Monday, November 24, 2008 @ 4:07 pm

Leave a comment