Working in the AssetsGraphed source code has been fun, and I want to take this chance to thank John Topley once again for submitting it. He’s done a great job, and it’s a great application, make no mistake of that. It takes a lot of courage to spread your code before all the world, and have people pick and poke at it. Thanks, John!

For this last article, I want to focus on a pattern that hearkens back to the Skinny Controller, Fat Model idea that I wrote about on my blog, back in October. The goal, here, is to slim down the following action from the IncomingController:

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

def create
  asset = Asset.find(session[:asset_id])
  incoming = Incoming.create
  incoming.asset_id = asset.id
  incoming.category_id = params[:incoming][:category_id]
  incoming.transaction_date = get_date(
    params[:incoming]['transaction_date(1i)'],
    params[:incoming]['transaction_date(2i)'],
    params[:incoming]['transaction_date(3i)'])
  incoming.amount = params[:incoming][:amount]
  incoming.balance = Balance.create(:asset_id => asset.id,
    :incoming_id => incoming.id,
    :transaction_date => incoming.transaction_date,
    :amount => asset.balance + incoming.amount)
  if incoming.save
    redirect_to asset_path(session[:username], asset.id)
  else
    flash[:notice] = "Amount #{incoming.errors.on(:amount)}"
    redirect_to :back
  end
rescue ArgumentError => exception
  case exception.to_s
    when 'invalid date'
      flash[:notice] = 'Please select a real date.'
    else
      flash[:notice] = 'Please enter a numeric amount.'
  end
  incoming.destroy
  redirect_to :back
end

I’m going to assume that the refactoring I recommended in Part 1 has been applied. That is to say, Incoming and Outgoing have been combined into a single model called Item. I’m also going to assume that the refactoring that Koz recommended in Part 2 has also been done, such that current_account is a method that always returns the account associated with the current session.

So, let’s take it a bit at a time. First, consider the first line:

asset = Asset.find(session[:asset_id])

This is referring to an asset-id that was stored in the session by the AssetController, in the show action. My advice here is to not do that. In general, store as little in the session as possible. Suppose someone wants to view two assets side-by-side, by opening them in separate windows? BOOM. They clobber each other, and you wind up with bizarre errors like incoming or outgoing items being added to the wrong asset.

Instead, encode the asset ID in the URL. You can do this easily with the RESTful routes like so:

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

# in config/routes.rb, this allows for routes like
# item_path(asset, item) --> "/assets/:asset_id/items/:id"
map.resources :assets do |assets|
  assets.resources :items
end

# in the view, you need both asset and item in order to build
# the URLs.
link_to("Item", item_path(@asset, @item)

# in the controller, you can use a before_filter to extract
# the @asset variable consistantly
class ItemsController < ApplicationController
  before_filter :find_asset

  #...

  private
    def find_asset
      @asset = current_account.assets.find(params[:asset_id])
    end
end

Now, you can rely on @asset existing wherever you need it to within that controller, and you can drop that first line of the #create action altogether.

For the next bit, we can collapse almost that entire method into a single create statement, as Koz explained in the last article:

@asset.items.create!(params[:item])

Note a few things about this. First, we don’t care about the returned item instance, since we never need to reference it. We just ignore it, resting comfortably in the knowledge that the record has been recorded in the database. Secondly, the get_date hokey-pokey that the original code performs is not necessary, since Rails will automatically detect and combine attributes named with the “(1i)” (etc.) suffix. Thirdly, by adding an after_create callback, you can take care of building and assigning the new Balance instance:

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

class Item < ActiveRecord::Base
  after_create :record_balance

  #...

  private
    def record_balance
      create_balance :asset_id => asset_id,
        :transaction_date => transaction_date,
        :amount => asset.balance + balance
    end
end

Finally, notice that I used the “bang” version of the #create method; this means that if any validation fails, a RecordInvalid exception is raised. We can use that to handle the case of an invalid record:

1
2
3

rescue ActiveRecord::RecordInvalid => error
  flash[:notice] = "Amount #{error.record.errors.on(:amount)}"
  redirect_to :back

Fortunately, RecordInvalid includes a record accessor that returns the record that failed validation, so we just reference that in the notice.

If everything works as expected and no exceptions are raised, we just redirect back to the containing asset:

redirect_to asset_path(session[:username], @asset)

Note that you don’t need to do @asset.id, since the routing code will call #to_param on each argument, and the default ActiveRecord::Base#to_param implementation just returns the id. It’s all about convention, baby. :)

So, the final, refactored version:

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

def create
  item = @asset.items.create!(params[:item])
  redirect_to asset_path(session[:username], @asset)
rescue ActiveRecord::RecordInvalid => error
  flash[:notice] = "Amount #{error.record.errors.on(:amount)}"
  redirect_to :back
rescue ArgumentError => exception
  if exception.to_s == 'invalid date'
    flash[:notice] = 'Please select a real date.'
  else
    flash[:notice] = 'Please enter a numeric amount.'
  end
  redirect_to :back
end

Much tighter and easier to read. This is a pretty powerful pattern, actually—once you wrap your mind around it, you’ll find it hard to go back to the fat controller philosophy. Not only is it more elegant, but it’s easier to test, since you can now test the entire item creation process independently of the controller. But that’s a whole ‘nother can o’ worms, and we won’t be going there today!

Thanks again, John, it’s been fun.