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?

Share/Bookmark