The Rails Way - all

This is part two in our series reviewing, John Topley’s AssetsGraphed.

One of the common anti-patterns I saw was the overuse of, and over-reliance on, the identifiers of objects. In general, unless it’s going into a url or the session, there’s very little reason for you to be passing around the id of an active record object, you should be using the object itself.

There are a few places in Assets Graphed which use IDs, but the most common is the get_account_id method in ApplicationController.

1
2
3

  def get_account_id
    session[:account_id]
  end

This is used all over the application, and it’s almost always passed straight to Account.find. A simpler, and probably more efficient, alternative is to make the application helper return the current_account instead of its ID, not only is this ‘more object oriented’, you can also use memoization to ensure you only issue one database query per request.

1
2
3

  def current_account
    @account ||= Account.find(session[:account_id])
  end

Now you can call current_account whenever you need the current account, instead of Account.find(get_account_id).

One extremely common (ab)use of IDs is using them for finding associated records. The FeedController contains the following code for finding the first 10 Incoming’s for a given Asset.

1
2
3
4
5
6

  def asset
    @asset = Asset.find_by_feed_url(params[:id])
    @incomings = Incoming.find(:all, :conditions => { :asset_id => @asset.id },
      :limit => 10)
    #...
  end

However, Active Record’s associations let you avoid this use of the id. To find the first 10 Incomings, simply ask the asset for them.

  @asset.incomings.find(:all, :limit=>10)

Once you start thinking about your methods in terms of the objects they relate to, instead of the opaque integers which identify them, several other refactorings become apparent.

Jamis says:

Always be on the lookout for duplicated code. If you find yourself doing asset.incomings.find(:all, :limit => 10) in multiple places, then perhaps you need to pull that into a method of its own. Given the use of the magic number “10�, it might not be a bad idea to do so anyway:

1
2
3
4
5
6
7

class Asset < ActiveRecord::Base
  has_many :incomings do
    def recent(count=10)
      find(:all, :limit => count)
    end
  end
end

Then, you just have to do asset.incomings.recent to get the first 10 items.

Another common use of IDs, is when manipulating belongs_to associations, for example the create method of the AssetController contains the following code.

1
2
3
4
5
6
7
8
9
10

  def create
    @asset = Asset.create(params[:asset])
    @asset.starting_balance = params[:asset][:starting_balance]
    @asset.account_id = get_account_id
    if @asset.save
      flash[:notice] = "Successfully added #{@asset.name}."
      redirect_to assets_path
    else
      render :action => :index
    end

If we transform it to use the account object instead of the ID, it will look like this:

  @asset.account = current_account

But once you’re dealing with the object itself, it should become apparent that we can use the association proxies to tidy up that code.

  @asset = current_account.assets.build params[:asset]

This lets you halve the code, and also ensures that the Asset appears in current_account.assets, guaranteeing a consistent in memory object graph.

For a final example we can take a look at the total_incomings method on the Asset class. The current implementation takes the ID of an account, and returns the ‘number of incomings’ across all its assets.

1
2
3
4
5
6
7
8
9

  def self.incomings_count(account_id)
    incomings_count = 0
    assets = Asset.find_all_by_account_id(account_id) or
      raise ActiveRecord::RecordNotFound
    assets.each do |asset|
      incomings_count += asset.incomings.count
    end
    incomings_count
  end

The first step is to refactor this to a method on an object instance, instead of a static method taking an identifier. The purpose of the method is to determine the “number of incomings� for an Account, so ideally we can call it with @account.number_of_incomings. If we move the implementation to the Account class, and throw in some Enumerable#inject magic we’re left with:

1
2
3
4
5

  class Account < ActiveRecord::Base
    def number_of_incomings
      assets.inject(0) {|total, asset| total += asset.incomings.size }
    end
  end

While this is functionally correct, it’s going to issue one database query for every Asset belonging to an account. This will put excessive load on the database, and it’s just plain wasteful. Thanks to has_many :through we can issue one database query to figure it out. First define the associations.

1
2
3
4

  class Account < ActiveRecord
    has_many :assets
    has_many :incomings, :through=>:assets
  end

Now you can get the number of incomings for an account the same way you would any other association. @account.incomings.size

So every time you find yourself accessing an object’s id, try to think if it’s really necessary, chances are there’s a nice opportunity to improve the design, and performance, of your application.

John Topley has been kind enough to submit his AssetsGraphed application to us for review. He even set up a publicly-accessible subversion repository for (a subset of) the code: http://johntopley.com/svn/projects/assetsgraphed/.

AssetsGraphed is, according to the site, “a simple asset tracking application that also tracks your data.� “Assets�, in this sense, is referring to financial assets, so you can keep track of your spending. Koz and I will be spending a few articles discussing better ways to approach some of the programming problems John has tackled in this application.

For this first article, I’d like to focus on how to refactor away some of the grosser kinds of code duplication. One of the first questions John asked us in his submission was about how to clean up the duplication in the “incoming� and “outgoing� controllers and models. (Compare them here: incoming and outgoing controllers; incoming and outgoing models.) As you can see, the two are almost completely word-for-word identical—a poster child for refactoring if there ever was one.

Let’s begin by looking at the models. In this case, the only textual difference between the two files is the name of the model:

1
2
3
4
5

$ diff app/models/incoming.rb app/models/outgoing.rb 
1c1
< class Incoming < ActiveRecord::Base
---
> class Outgoing < ActiveRecord::Base

Given that the two models have that level of similarity, my first suspicion was that they were really the same model, simply considered from different directions. What if, instead of Incoming and Outgoing models to track “money you are getting� and “money you are spending�, we simply defined a single “Item� model that encapsulated all transactions?

The differentiator between “incoming� and “outgoing� transactions then simply becomes the sign on the amount. “Incoming� transactions are positive numbers, and “outgoing� transactions are negative. As long as an index exists that includes the “base_amount� column, queries that ask for “base_amount < 0� and “base_amount >= 0� should be plenty efficient.

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

class DefineItemsTable < ActiveRecord::Migration
  def self.up
    create_table :items do |t|
      t.column "asset_id",         :integer
      t.column "category_id",      :integer
      t.column "transaction_date", :date
      t.column "created_on",       :datetime
      t.column "base_amount",      :integer
    end

    # for finding all items by asset
    create_index :items, %w(asset_id base_amount)

    # for finding all items by category
    create_index :items, %w(category_id base_amount)
  end
end

(That second index, for finding all items by category, may be superfluous, unless the application needs to retrieve items by category.)

Once that new table and model is in place, we can add some methods to the Asset class for returning the “incoming� and “outgoing� items:

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

class Asset < ActiveRecord::Base
  # all items, incoming and outgoing
  has_many :items, :dependent => :destroy_all do
    def incoming
      @incoming ||= find(:all, :conditions => "base_amount > 0")
    end

    def outgoing
      @outgoing ||= find(:all, :conditions => "base_amount < 0")
    end
  end
end

The “do…end� syntax on the has_many association simply extends the association with an anonymous module containing the given methods. In other words, we can now do something like this:

1
2
3
4
5
6
7
8
9
10
11

# given some asset...
asset = Asset.find(params[:id])

# we can return the list of _all_ items...
asset.items

# or only the incoming items...
asset.items.incoming

# or only the outgoing items...
asset.items.outgoing

Koz says:

An alternative to the anonymous modules is to simply declare two more associations.

1
2
3
4
5

class Asset < ActiveRecord::Base
  has_many :items, :dependent => :destroy_all 
  has_many :incoming_items, :class_name => 'Item', :conditions => "base_amount > 0"
  has_many :outgoing_items, :class_name => 'Item', :conditions => "base_amount < 0"
end

It’s a little simpler to understand, and clocks in at a whopping 6 fewer lines of code. The only difference is that instead of accessing the incoming items with @asset.items.incoming, you’d use @asset.incoming_items.

Once the models are unified, the task of unifying the duplicated controllers follows naturally, resulting in a single ItemsController. The client must simply submit the item amount as either positive (for incoming) or negative (for outgoing), and the categorization occurs automatically. If there is a concern about requiring users to enter negative numbers, you can pass an extra parameter to the create action that indicates whether the number should be negative or not.

I really love extreme refactorings like this—there is very little in programming that gives me as much pleasure as axing code.

Todd Huss has submitted the file import code for Wind and Tides. Periodically the site needs to fetch data from the NOAA and update the local database. Here’s his import code:

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

class Wind < ActiveRecord::Base

  validates_presence_of :created, :wind_location_id
  
  belongs_to :wind_location
  
  # Get all of the wind locations, then fetch, parse, and save the data
  def self.batch_update!(wind_locations = WindLocation.all)
    wind_locations.each do | wind_location |
      raw_noaa_data = Net::HTTP.get_response(URI.parse(wind_location.url)).body
      raise "Received unexpected wind response from NOAA: " + raw_noaa_data unless raw_noaa_data=~/TIME/
      save_wind!(raw_noaa_data, wind_location)
    end
  end
  
  # Take a text based NOAA forecast, parse it, populate the model objects, and save it
  def self.save_wind!(noaa_text, wind_location)
    # ...
  end      

  # NOAA publishes 181809Z for Wind which we need to turn into YEAR/MONTH/18 18:09 UTC
  def self.convert_noaa_to_time(noaa_time, now = Time.now.getutc)
    # ...
  end 
end

There’s one major structural change I’d make, and a few minor enhancements.

The structural change I’d make is to move the downloading and parsing code out of the model and into a utility class. This lets you keep your models focused on your application, and have the file import code tucked away in the lib directory. At the same time, if we tie the importer to a single WindLocation and remove the loop in the batch_update! method, we make the focus tighter:

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

class WindImporter
  class << self
    def batch_update!(wind_location)
      raw_noaa_data = Net::HTTP.get_response(URI.parse(wind_location.url)).body
      raise "Received unexpected wind response from NOAA: " + raw_noaa_data unless raw_noaa_data=~/TIME/
      save_wind!(raw_noaa_data, wind_location)
    end
    
    # Take a text based NOAA forecast, parse it, populate the model objects, and save it
    def save_wind!(noaa_text, wind_location)
      # ...
    end      
    
    # NOAA publishes 181809Z for Wind which we need to turn into YEAR/MONTH/18 18:09 UTC
    def convert_noaa_to_time(noaa_time, now = Time.now.getutc)
      # ...
    end
  end
end

class WindLocation < ActiveRecord::Base
  has_many :winds
  def batch_update!
    WindImporter.batch_update!(self)
  end
end

By making WindLocation#batch_update! delegate to the importer, we can work with it in a more object oriented manner. (Also, using delegation makes it possible to use a “mock� object for the importer during testing.)

1
2
3
4
5

  # Import the files for one location
  @wind_location.batch_update!

  # or, import them all
  WindLocation.find(:all).each(&:batch_update!)

With that change out of the way, we can make a few enhancements to the implementation of some of the methods. batch_update! has a big chunk of net/http boilerplate which can be replaced by a tiny piece of open-uri.

1
2
3
4
5
6

  require 'open-uri'
 
  def batch_update!(wind_location)
    noaa_text = open(wind_location.url).read
    # ...
  end

There are a few snippets which can be simplified in save_wind! too. Here’s the original:

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

   def self.save_wind!(noaa_text, wind_location)
    # Filter and append lines with data
    lines = Array.new
    noaa_text.each_line do | line |
      next unless line=~/^\d\d\d\d\d\dZ/
      lines << line
    end
    raise "Received unexpected wind report from NOAA: " + noaa_text unless lines.length > 0    
    # Process each line in reverse order (oldest first)
    lines.reverse_each do | line |
      created = convert_noaa_to_time(line[0,6])
      wind = Wind.find_or_create_by_created_and_wind_location_id(created, wind_location.id)      
      class << data = line.split
        # Convert field to int unless it's "--" then nil
        def to_i(index)          
          (self[index]=~/^-+/)?nil:self[index].to_i
        end
      end 
      wind.direction = data.to_i(wind_location.position)
      wind.sustained = data.to_i(wind_location.position + 1)
      wind.peak = data.to_i(wind_location.position + 2)
      wind.save!
    end
  end      

The initialization of lines can be done in one line by using Array#grep and a slightly simplified regular expression. (Array#grep simply returns all elements that match the given regex.)

  lines = noaa_text.grep(/^\d{6}Z/)

Next, instead of calling the unwieldy find_or_create_by_created_and_wind_location_id we can use class methods on associations to initialize wind. (For more about class methods on associations, you can read the article I wrote when we were dissecting the Tracks application.)

  wind = wind_location.winds.find_or_create_by_created(created)

Finally we can simplify the code which converts integers. In the NOAA format, �—� represents nil, and other integers are as expected. While the original code converted the data by defining a method on the data array, we can instead create a new array containing just the integers with Enumerable#map. After creating the new array, we can safely retrieve the figures.

1
2
3
4
5
6

   data    = line.split.map{|d| d.starts_with?("--") ? nil : d.to_i }
   pos     = wind_location.position
        
   wind.direction = data[pos]
   wind.sustained = data[pos + 1]
   wind.peak      = data[pos + 2]

Jamis says:

Another option for parsing integers is to use the Integer method. It will raise an exception if the argument cannot be converted, so you could just do something like:

  data = line.split.map{ |d| Integer(d) rescue nil }

That won’t work, of course, if you want any non-integer value besides �—� to map to 0, but it does reduce the line-noise a bit.

This makes the code much easier to follow and a little more explicit. The fully refactored save_wind! is presented below:

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

    def save_wind!(noaa_text, wind_location)
      lines = noaa_text.grep(/^\d{6}Z/)
      raise "Received unexpected wind report from NOAA: " + noaa_text unless lines.length > 0    
    
      # Process each line in reverse order (oldest first)
      lines.reverse_each do | line |
        created = convert_noaa_to_time(line[0,6])
        wind    = wind_location.winds.find_or_create_by_created(created)
        data    = line.split.map{|d| d.starts_with?("--") ? nil : d.to_i }
        pos     = wind_location.position
        
        wind.direction = data[pos]
        wind.sustained = data[pos + 1]
        wind.peak      = data[pos + 2]
        wind.save!
      end
    end      

Thanks again to Todd for making the submission.

Dan Hancu sent us a nice, bite-sized snippet of code, asking if there was a more elegant way to do what he was doing. His code was from a controller, part of a larger application that included a system for tracking an inventory of lenses. When creating a new type of lens, you just input a few parameters and the system then autogenerates all relevant lenses for that type of lens.

His code (in essense) is:

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
33

class TypeOfLensController < ApplicationController
  def create
    @type_of_lens = TypeOfLens.new(params[:type_of_lens])
    if @type_of_lens.save
      flash[:notice] = 'TypeOfLens was successfully created.'
      populate(@type_of_lens)
      redirect_to :action => 'list'
    else
      render :action => 'new'
    end
  end

  private
  
    def populate(type_of_lens)
      x_count = type_of_lens.minimum_cylinder
      y_count = type_of_lens.maximum_sphere
      increment_step_size = 0.25
      while y_count >= type_of_lens.minimum_sphere
        while x_count <= type_of_lens.maximum_cylinder
          lens = Lens.new
          lens.lens_type_id = type_of_lens.id
          lens.sphere = y_count
          lens.cylinder = x_count
          lens.quantity = 0
          lens.save
          x_count += increment_step_size
        end
        x_count = type_of_lens.minimum_cylinder
        y_count -= increment_step_size
      end
    end
end

Now, naturally, since I’m not acquainted with the domain of the application in question, it’s hard for me to judge the bigger picture, such as whether the model associations and such are appropriate or not. I’m going to assume that Dan, who is much more acquainted with the domain than myself, did all that right, and I’ll just focus on some of the smaller mechanical details of the code in question.

First off, the populate method really ought to be a method on TypeOfLens, called via an after_create hook:

1
2
3
4
5
6
7

class TypeOfLens < ActiveRecord::Base
  after_create :populate

  def populate
    #...
  end
end

By setting up populate as an after_create hook, we can rest easily knowing that every time a new TypeOfLens is instantiated, that populate method will be auto-invoked. That knowledge lets us simplify the controller significantly:

1
2
3
4
5
6
7
8
9
10
11

class TypeOfLensController < ApplicationController
  def create
    @type_of_lens = TypeOfLens.new(params[:type_of_lens])
    if @type_of_lens.save
      flash[:notice] = 'TypeOfLens was successfully created.'
      redirect_to :action => "list"
    else
      render :action => "new"
    end
  end
end

Now, we can turn our attention to the populate method itself. Although Dan’s original code certainly suffices, it has the flavor of C code, simply modified to use basic Ruby syntax. By taking advantage of standard Ruby methods and patterns, we can not only compact the code, we can also make it look like idiomatic Ruby code instead of C code.

First of all, note the loop idiom being used:

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

def populate(type_of_lens)
  x_count = type_of_lens.minimum_cylinder
  y_count = type_of_lens.maximum_sphere
  increment_step_size = 0.25
  while y_count >= type_of_lens.minimum_sphere
    while x_count <= type_of_lens.maximum_cylinder
      #...
      x_count += increment_step_size
    end
    x_count = type_of_lens.minimum_cylinder
    y_count -= increment_step_size
  end
end

All this is doing is iterating from some upper bound, maximum_sphere, to a lower bound, minimum_sphere, by some constant increment. The inner loop does the same thing, from a lower bound to an upper bound. Ruby, predictably, has a method for that specific use case, Numeric#step:

1
2
3
4
5
6
7
8
9

STEP_SIZE = 0.25

def populate
  maximum_sphere.step(minimum_sphere, -STEP_SIZE) do |sphere|
    minimum_cylinder.step(maximum_cylinder, STEP_SIZE) do |cylinder|
      #...
    end
  end
end

The Numeric#step method simply calls the associated block once for every increment of the second parameter, from the receiver to the first parameter. In other words, it “steps� (in the case of the outer loop) from maximum_sphere to minimum_sphere in increments of -STEP_SIZE.

Lastly, let’s address the process of creating a new Lens instance. In Dan’s original code, this was done by first creating an empty Lens object, then assigning the relevant attributes one-by-one, and saving:

1
2
3
4
5
6

lens = Lens.new
lens.lens_type_id = type_of_lens.id
lens.sphere = y_count
lens.cylinder = x_count
lens.quantity = 0
lens.save

The Rails way of doing this employs the create method, passing it a hash of the attributes you wish the new object to have. It then returns the newly created (and saved) object. In our version, we ignore the return value, since it is not relevant to what we’re doing—it suffices us to know that the Lens object was created.

1
2

Lens.create :lens_type_id => type_of_lens.id,
  :sphere => y_count, :cylinder => cylinder, :quantity => 0

The final version of the TypeOfLens model:

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

class TypeOfLens < ActiveRecord::Base
  STEP_SIZE = 0.25

  after_create :populate

  # ...
  def populate
    maximum_sphere.step(minimum_sphere, -STEP_SIZE) do |sphere|
      minimum_cylinder.step(maximum_cylinder, STEP_SIZE) do |cylinder|
        Lens.create :lens_type_id => id,
          :sphere => sphere, :cylinder => cylinder, :quantity => 0
      end
    end
  end
  # ...
end

In the end, I think this exercise illustrates nicely the value of learning Ruby, as well as Rails. Certainly, we all must start some place, and Rails offers plenty to learn for newcomers. But once you know the basics, it pays in a big way to keep stretching, learning Ruby idioms for common patterns.

The Rails way, after all, is only a specialized subset of the greater Ruby way.

This is the fourth and final article in the series reviewing the GTD application Tracks.

Rails is a wonderful productivity enhancer. It can sometimes seem almost like magic: you write a few lines of code here and there, and Rails just “does the right thing.� What’s not to love about that?

Unfortunately, that magic often tricks newcomers to Rails into thinking that the right thing will be done, even when there is no way for Rails to do so. I’m speaking, specifically, of database indexes (or rather, the lack thereof).

The Tracks application is certainly not the only one to do this, so this is in no way directed solely at them. I’ve encountered far too many Rails applications that seem to assume that defining the database tables is sufficient, and that Rails will magically take care of the rest. This, sadly, is not true, and has resulted in a whole generation of web applications with terrible database performance. The only indexes that Rails adds are on your tables’ primary keys. All other indexes must be defined explicitly, by you.

Even veterans can make this mistake. At 37signals, we were missing indexes on four (smaller) tables in Campfire until recently. It wasn’t until we started wondering why the load on that database was so high that I went looking, and discovered some full-table scans occuring in some frequent queries. Adding the necessary indexes caused the load to drop by over half!

I’ve addressed the question of when to add indexes in a post on my own blog. Rather than repost that article here, you can read it there: Indexing for DB performance.

In general, you’ll want indexes on your foreign keys, but if it were only that simple, Rails could figure that out for you. Sadly, the indexes you need depend largely upon how your application will query the database. Consider the following table definition:

1
2
3
4
5
6
7

create_table "notes", :force => true do |t|
  t.column "user_id", :integer, :default => 0, :null => false
  t.column "project_id", :integer, :default => 0, :null => false
  t.column "body", :text
  t.column "created_at", :datetime
  t.column "updated_at", :datetime
end

The foreign keys here are user_id and project_id. Should there be indexes on those? It depends.

Consider the SQL generated for the following call:

note = Note.find(1234, :include => :user)

You should see SQL something like the following in your log:

1
2
3
4

SELECT *
  FROM notes, users
 WHERE users.id = notes.user_id
   AND notes.id = 1234

The database, upon receiving this query, has to determine how to satisfy it. Without getting into the nitty-gritty (mostly because the nitty-gritty differs between databases, but also because I’m only superficially familiar with the nitty-gritty, anyway), the DB decides that it will start with grabbing the note, since it was given a constant for that primary key.

So, it knows the note. That means it knows the user_id, and can then satisfy that part of the query via the primary key on users. After that, it’s done, since it has found all the necessary rows.

In this case, it was able to satisfy the query via the primary keys on the two tables. No other indexes were necessary, and since Rails defines our primary keys for us, that kind of query works efficiently “out of the box�.

Now, consider the SQL generated for something like this:

1
2

# user = User.find(1234)
notes = user.notes

You’ll see something like this in your logs:

1
2
3

SELECT *
  FROM notes
 WHERE user_id = 1234

Ok, in this case, we know the user_id, since we know the user the notes belong to. We want to find all notes with that user_id. The database, however, cannot satisfy that query using any of the defined indexes, so it has to resort to the dreaded “full table scan�. It has to look at every row in the notes table. For small tables, even up to a few thousand rows, that won’t be noticable. But when you start getting tens or hundreds of thousands of rows, the difference becomes significant. If you’re on a shared host, people will start hating you.

So, let’s assume you recognize that your application is querying like this, looking records up by foreign key. Rails, fortunately, makes it simple to add new indexes:

1
2
3
4
5
6
7
8
9
10

# script/generate migration AddIndexToUser
class AddIndexToUser < ActiveRecord::Migration
  def self.up
    add_index :notes, :user_id
  end

  def self.down
    remove_index :notes, :user_id
  end
end

In general, you’ll want an index on any combination of columns that you are using to look up records. For Tracks, “Find all notes for this user� needs an index on the foreign key, but “Find the user for this note� does not.

I really cannot stress enough the importance of getting to know how your DBMS of choice works. I’ve only scratched the surface in this article, and in the post on my own blog. Find a good book on SQL and indexes. Learn specifically how your database uses indexes to satisfy queries. You cannot hope to scale your application, in any framework, if your database queries are doing full table scans all the time.

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.

Here we are, #2 in the series of articles exploring the GTD application Tracks. In part 1, Koz discussed the finer points of code reuse. Here, I’ll talk about action filters in your controllers.

I’m focusing primarily on the ContextController and ProjectController in this article—both have many similarities. Their actual purpose is not really relevant here, so I won’t describe what they actually do. Rather, let me just point out one of the first things I noticed when I started reading through the code. This pattern was startlingly prevalent:

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

def list
  init
  ...
end

def show
  init
  init_todos
  ...
end

def create
  ...
end

def add_item
  self.init
  ...
end

Note that the same method gets called at the start of many of these actions. The intent here, is to do some kind of common setup, or initialization, prior to the meat of each action. Rails provides a cleaner way to do this, via filters. Filters are simply methods that get called before or after an action, and this code is a textbook example of when you ought to be using them.

In general, filters that you want executed before an action are defined via the before_filter macro:

before_filter :init, :init_todos

Defining the filter like that would cause both init and init_todos to be called prior to every action in the current controller (and its subclasses, since filters in a superclass are inherited by subclasses).

What we want in this instance is something a bit more sophisticated. We don’t want the filters to trigger on every action, only on some of them. In fact, there are only one or two actions that need the init_todos filter. Fortunately, before_filter (and the filter macros in general) provide a very simple, but very powerful, mechanism for including or excluding actions:

1
2

before_filter :init, :except => :create
before_filter :init_todos, :only => :show

The above specifies that the init method should be called prior to every action, except create, and init_todos should be called prior only to the show action. Both except and only allow arrays of action names to be given, as well.

Using filters like this can help you to reduce the “noise� in your actions, leaving you free to implement only the tasty bits within the action itself. It really makes your code easier to read, and to maintain.

Another (related) thing I noticed, this time in the ApplicationController, was the following:

1
2
3
4
5
6
7
8

before_filter :set_session_expiration

# ...

def set_session_expiration
  return if @controller_name == 'feed' || ...
  # ...
end

The top-level ApplicationController specifies a before filter, set_session_expiration, that will trigger on every single action in all subclasses. It has some logic to determine what the session’s expiration should be. Note, though, that if the controller is determined to be the FeedController (the name of the controller is “feed�), that it just returns.

In general, if you need to refer to the current controller or action name explicitly in your code, you could probably refactor it to be cleaner. In this case, you can let Rails’ filtration code do the conditional work for you. In ApplicationController, simply omit that condition altogether. Then, in the FeedController, do:

1
2
3
4
5

class FeedController < ApplicationController
  skip_before_filter :set_session_expiration

  # ...
end

Koz says: The reality is most aggregators don’t work with sessions anyway, what you probably want in FeedController is session :off. This will disable sessions for that controller, and stop your sessions table from filling up with empty, aggregator-generated sessions.

The skip_before_filter macro allows subclasses to override filter behavior configured in their parent class. Here, we tell Rails that we want to “skip�, or ignore, the set_session_expiration filter for all actions in this controller. (You can use the only and except parameters here, as well, if you need finer tuning.) By using the skip_before_filter macro, the code that applies to the FeedController is in the FeedController, and the ApplicationController remains blissfully unaware of its subclasses.

(While we’re on the topic of filters, I might just add that filters in edge rails have one minor drawback: they cause your stacktrace to become ridiculously long, because they are processed recursively. A way to process them without making recursive calls would be a wonderful patch. Hint, hint!)

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 /lib/acts_as_namepart_finder.rb which lets you find an object by its ‘name’, and falls back to a partial match; and 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

Jamis says: The simplest way to do this is to just monkeypatch ActiveRecord::Base directly, via config/environment.rb. However, if you find yourself extending classes more than once or twice, it’s nice to keep that file clean by putting your extensions in the lib directory. Just make sure you require those files explicitly from config/environment, to make sure your extensions get loaded.

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

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.

Doing something for the first time is always an educational experience. Koz and I are just about ready with the first installment of The Rails Way, and in preparing it we learned a few things.

Most significantly, we learned that multiple short articles are preferable to one lengthy article.

With that nugget of wisdom in mind, tomorrow we will unleash the first in a series of four articles reviewing the GTD application Tracks. Luke Melia’s submission was one of about a dozen that we recieved—quite a few more than we expected for this first run of The Rails Way!

We were drawn right away to the Tracks submission for a few reasons. First, Koz had used it before and was somewhat familiar with it. Second, upon perusing the code, it illustrated a few common errors and antipatterns that we thought would be helpful to address in this first series of posts.

Note that we are not “picking on� Tracks here. The issues we will address are definitely not unique to Tracks. Koz and I have both seen them in numerous other projects, which is why we’re talking about them.

We extend our thanks to Luke and to everyone else who sent us code, for their faith in us and in this blog. We’ve been quite overwhelmed by the positive response to our original announcement: two weeks and zero content later, and there are already nearly a thousand subscribers to the RSS feed! We hope this series and the many series to come will prove that faith to be well-founded.

So, brace yourselves! Koz will be posting the first of the articles reviewing Tracks in just a few hours.

Welcome to The Rails Way. We are Jamis Buck and Michael Koziarski.

With all the attention Rails has received in the last year, there has been an enormous influx of people wanting to learn how to write web applications with it. Rails itself attempts to shepherd programmers along well-trod paths, to help them build well-designed applications “by default�. Alas, Rails’ conventions are not always enough.

This blog is our attempt to back up Rails’ opinions on what things should be done, with our own opinions on how things should be done.

Here’s how we’ll do it: if you have an application or a piece of code which you think could be done better, but you aren’t quite sure how, send it in. We’ll review all submissions, then publish an article or two showing the changes we’d make, and explain why we’d make them.

Readers get the chance to learn tips and tricks for enhancing your design, submitters get their application refactored for free, and we get some free-inspiration for our blog posts. Nobody loses!

Please note, however:

If you send us code, we’ll assume we have permission to publish here. Don’t send urgent requests for help. We cannot guarantee timely responses to submissions, or even that we’ll publish your submission at all. If the problem your code solves can’t be described in a few sentences, it’s probably going to be hard for us to help you in a single blog post. Rails code is preferred, but if you send us a particularly fascinating (and smallish) project in another language, we may consider an article or two showing how it could be done better in Rails.

So, if you’re interested, please send your submissions to submissions@therailsway.com. We’ll take a few weeks initially to gather the submissions, choose one, and write about the process of improving it. We aren’t sure yet what our ideal schedule will be once this is rolling, but we hope to post these refactoring articles “regularly�.