AssetsGraphed: Part 2

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.