Tracks: Part 3

This is the third of four parts in our series reviewing Tracks.

The last thing I wanted to comment on was Tracks’ use of Inheritance (a.k.a. STI). Rails lets you easily map an inheritance hierarchy to a single table using Single Table Inheritance. All you need to do is create a column called type and rails will take care of everything for you. An unfortunate side effect of making inheritance so easy, is that people use it for a variety of situations where it doesn’t make sense. Ask any of my Rails Core colleagues and they’ll tell you that STI abuse is my #1 Pet Peeve.

So lets take a look at the current tracks code.

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

class Todo < ActiveRecord::Base
  # ...
end

class Immediate < Todo
end

class Deferred < Todo
  validates_presence_of :show_from
  
  def validate
    if show_from != nil && show_from < Date.today()
      errors.add("Show From", "must be a date in the future.")
    end
  end
end

From my understanding of the application, the intention is that a user can take a Todo and ‘defer’ it until some date in the future. At that point, the Todo should start showing up on the main lists again. To achieve this, tracks uses Inheritance with two subclasses of Todo:

Deferred – For tasks which have been shelved until some future date Immediate – For tasks which should be done immediately

The main problem with using inheritance for this is that the you can’t, without serious hackery, change the class of an object, and Todo objects can be deferred at any time. Consider these two snippets; one of them should look completely wrong to you.

1
2
3
4
5
6
7
8

  # clearly an abomination, which is why it's impossible in ruby.
  an_integer = 12
  an_integer.class=String
  a_string = an_integer

  # this is what you'd expect
  an_integer = 12
  a_string = an_integer.to_s

If you find yourself manually updating the type attribute of an ActiveRecord model, you should step back, and reconsider your design: there’s no reason you should ever need to do that. Things in real life don’t magically transmogrify themselves, your model objects shouldn’t either.

A simple alternative is to add a deferred_until attribute to the the Todo table. If the deferred_until date is null, or in the past, a Todo is active, and if it’s in the future it’s deferred.

1
2
3
4
5
6
7
8
9

class Todo < ActiveRecord::Base
  def deferred?
    deferred_until && deferred_until > Time.now
  end

  def active
    !deferred?
  end
end

That gives you the ability to detect whether a Todo is deferred or active, but nine times out of ten you’ll want to split the Todos into two seperate lists. A naive implementation could consist of something like:

  @active_todos, @deferred_todos = @project.todos.partition {|t| t.active? }

But that’s really wasteful if you’re just looking for the active Todos. There are a few options to choose between. The easiest is to simply declare multiple associations. This gives you all the lazy-loading goodness which comes with an association, and accessing it is simple and unsurprising.

1
2
3
4
5
6
7

class Project < ActiveRecord::Base 
  has_many :todos
  has_many :active_todos, :class_name=>"Todo", 
                 :conditions=>"deferred_until is null or deferred_until < NOW()"
  has_many :deferred_todos, :class_name=>"Todo", 
                 :conditions=>"deferred_until > NOW() "
end

Jamis says: You might want to avoid timezone-specific times in your DB. Try setting the default ActiveRecord timezone to :utc, and use Time.now.utc instead of Time.now when storing times. This simplifies TZ support in your app, and avoids issues when your server is in a different zone.

As nice as that option is, it suffers from two drawbacks.

First, it’s not database agnostic (the “NOW� function is named differently in just about every DBMS). This may not matter for most projects, but it can be a deal-breaker for a downloadable application like Tracks. If you don’t have control over the database that your users will want to configure with your application, you’ll need to avoid using database-specific features.

The second drawback: it’s ugly as hell. The Project and Context classes contain the logic about deferring tasks, when it should be nicely encapsulated in the Todo class. Thankfully, class methods on associations can help us here.

We need to define two class methods on Todo that handle the searching:

1
2
3
4
5
6
7
8
9
10

class Todo < ActiveRecord::Base
  #...
  def self.deferred
    find(:all, :conditions=>["deferred_until > ?", Time.now])
  end

  def self.active
    find(:all, :conditions=>["deferred_until is null or deferred_until < ?", Time.now])
  end
end

Now, everyone knows that this lets us call Todo.deferred to get a list of all deferred Todos across the whole system, but that’s not really that useful. What we really want is to find all the Todos for a given project or context, which match the other conditions. What some readers may not know is that ActiveRecord lets you do this:

  @project.todos.deferred # ensures the Todo is deferred *and* belongs to the project

That leaves you with what you’re after: you can get deferred todos, active ones, and also get access to all the Todos by calling @project.todos. The logic is nicely encapsulated in the Todo class, and it works across all supported databases.

Update: Tracks has already updated their design to remove inheritance from their Todo classes, the design is slightly different to what I proposed here, but still a nice improvement.