Tracks: Part 1

This is part 1 in our multi-part series evaluating the funky GTD app Tracks.

Tracks has two files in their lib directory which got my attention.

• acts_as_name_part_finder

• acts_as_todo_container

/lib/acts_as_todo_container.rb which contains a lot of the logic for Todo List management.

These both illustrate a common anti-pattern I see with rails programmers: premature extraction. Just because rails has a bunch of meta programming magic with names like acts_as_list, doesn’t mean you need it.

We’ll start with acts_as_name_part_finder as it’s the simplest case. As the generated method is always the same, there’s no need for the indirection and complication which comes from using an ‘acts_as’ macro.

1
2
3
4
5
6

module NamePartFinder
  def find_by_namepart(namepart)
    find_by_name(namepart) || find(:first, :conditions => 
        ["name LIKE ?", namepart + '%'])
  end
end

You just have to call extend in your class definition. Once you’ve done that, you’ll get the nice find_by_namepart methods you’re used to using.

1
2
3

class Project < ActiveRecord::Base
  extend NamePartFinder
end

If you find yourself wanting this same behaviour for fields other than name, this solution won’t help at all. An alternative is to use a simple macro to generate some static methods for you. You can just stick this at the bottom of environment.rb, or somewhere else that gets required.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21

# config/environment.rb
def partial_finders_for(*attribute_names)
  attribute_names.each do |attribute_name|
    class_eval <<-EOS
      def self.find_by_partial_#{attribute_name}(attribute_value)
        find_by_#{attribute_name}(attribute_value) || find(:first, 
                :conditions=>["#{attribute_name} LIKE ?", attribute_value + '%'])
      end
    EOS
  end
end

# app/models/project.rb
class Project < ActiveRecord::Base
  partial_finders_for :name, :nickname
end

# app/controllers/project_controller.rb
def index
  @projects = Project.find_by_partial_name("jo")
end

But the more fundamental question here is whether to bother with a module at all. It seems the partial_name code is only ever called from one action, so my advice to you is, put it back inline and don’t worry about the duplication until you’re actually using it in multiple places. The only thing worse than repetitive code, is prematurely-extracted code.

Replacing a simple, working solution with a creaking abstraction is much worse than a few duplicated lines. DRY is about avoiding bugs, it’s not an absolute rule.

Now acts_as_todo_container is a slightly more interesting case, unlike name_part_finder. Because the current implementation requires a parameter, you can’t just use a simple module. Looking at the implementation, there are two things which jump out at me.

First, there’s a lot of boilerplate code to support ‘find_todos_include’, which is only needed to prevent the views from triggering multiple queries. Thankfully, has_many takes an :include option which can save us a lot of duplication.

1
2
3
4
5
6
7
8
9

class Context < ActiveRecord::Base
  has_many :todos, :dependent => :delete_all, :include=>:project
  #...
end

class Project < ActiveRecord::Base
  has_many :todos, :dependent => :delete_all, :include=>:context
  #...
end

Now whenever you access the ‘todos’ collection, AR will include the project or context as needed. With this refactoring in place we can simplify acts_as_todo_container to just one module.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32

module TodoList

  def not_done_todos
    @not_done_todos ||= find_not_done_todos
  end

  def done_todos
    @done_todos ||= find_done_todos
  end

  def find_not_done_todos
    self.todos.find(:all,
                    :conditions =>
                         ["todos.type = ? and todos.done = ?", "Immediate", false],
                    :order => 
                      "todos.due IS NULL, todos.due ASC, todos.created_at ASC")

  end

  def find_done_todos
    self.todos.find(:all, 
                    :conditions => 
                      ["todos.type = ? AND todos.done = ?", "Immediate", true],
                    :order => "completed DESC", :limit => @user.preference.show_number_completed)
  end
end


class Project < ActiveRecord::Base
  has_many :todos, :dependent => :delete_all, :include=>:context
  include TodoList
end

Another option would be to use composition and extract the Todo list behaviour into a specific class TodoList. Once you’ve done that you can probably simplify the rendering code a little as all the partials could focus on rendering a ‘todo list’.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

class Project < ActiveRecord::Base
  has_one :todo_list
  #...
end

class Context < ActiveRecord::Base
  has_one :todo_list
  # ...
end

class TodoList < ActiveRecord::Base
  has_many :todos, :dependent => :delete_all
  # ...
end

The main reason this would prove problematic is that Todo instances need to have references to both their context, and their project. By adding another class here, you’d need to manually ensure that you moved them to the relevant lists whenever you changed the belongs_to associations. That’s probably going to be a lot more code than just using the module method outlined above.