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.
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.
The Rails Way is all about teaching "best practices"
in 
Really amazing guys! Thank you very much, this website is my best resource for rails tips and suggestion.
Great post guys. This information really needs to get further out into the Rails community.
I think with some of this stuff I was so busy trying to learn Ruby and Rails that I couldn’t see the forest for the trees when it came to good OO design. The great thing about having your code reviewed on this site is that you’re effectively getting top-notch Rails consultancy for free. The worst thing is that you can look like a bumbling amateur! ;-)
There’s probably nothing wrong with it, but the fact that account.incoming is mentioned three times with three different uses puzzled me. Is it :through, does it have extensions, etc? Great post, this site is a terrific resource.
Thanks for the post.
Wouldn’t it be handy to include, on this page, a link back to part one of this article? Perhaps near the top of the article where I’ll first expect to find it?
Or is my vision failing me?
Thanks for the great tips.
Every time I read an article here, I instantly have an urge to refactor my code. Your articles make me look dumb but that’s a good sign. The alternative is to be dumb all the time. Thanks a bunch.
@asset = current_account.assets.build params[:asset]From the looks of it, I’m guessing ‘build’ is similar to ‘create’, but used in an associative way. I’ve been doing something similar, but instead I typically do: The Rails API doesn’t have any comments for ‘build’, but if this is all it does, it’d save me some extra lines here and there.@ Jason L. – Yes it does have docs. Look at docs for has_many relation. Build is pretty much similar to new but it does something more. It fills the foreign key for you, so in this case @asset.account_id is already set to the @account.id. So while doing @asset.save the model is already linked to the parent.
Ah, thanks Piotr. Looking back at some actual code, I see I’ve been doing another extra step when I use new (setting the foreign key by doing something like
@asset.account = current_account), or in other cases I’ve been using ‘create’ which sets the FK as well (when doing@asset = current_account.assets.new params[:asset]).Very good to know – I’m sure that has caused me trouble in the past, where I’ve expected ‘new’ to set the FK and I should be using ‘build’ instead.
Trevor,
Good point, I’ll update our ‘series’ sidebar to include these ones.
Koz, great article! By reading your post I actually found out that I had the opposite problem on my app, I kept the object itself at @session[:client] and I realized now that, if one user updates the object, the other users would not see the change. Of course this also made me realize that I have tons of @session[:client] calls all over my controllers and views that are screaming for a helper function.
Congratulations for all the articles, there wasn’t a single one where I didn’t learn something new that I could use on my own projects.
Interesting! Just a small question: what’s the lifetime of a memoization? Is the state kept for the lifetime of 1 request? What about using it with AJAX stuff?
Thanks for your great hints! Bart
The memoization lasts for as long as the instance variable lives, which (unless you manually reset it somehow) is as long as the object lives. Rails reinstantiates all controllers and models on every requests, but does not reload the classes (in production mode), so be careful not to memoize on class methods unless you really mean them to remain constant for the life of the process.
So the way you implemented the current_account method it will not make any computation after the first call (when the computation is “cached”) and the beginning of a new request? I find that a really interesting approach as I am working on a project with a similar performance hit where checking on logged in users and their rights requires DB access. But it could be cached for just one request this way, right? (I know I’m going to end up sending you the entire code, but I want to try and improve the codebase myself first, based on your posts.)
Bart,
Yep, if you memoize in your current_account method you will only do a single database query per request.
The last code example has this association:
has_many :incomings, :through=>:assets
What should that association be if using the first refactoring which saw the Incoming and Outgoing models replaced with an Item model?
John, you would specify the name of the collection on Asset that you want to query. In other words, if you now have “has_many :items” on Asset, you’ll have “has_many :items, :through => :assets” on Account.
Jamis,
That’s what I thought.
However, that doesn’t let me get a count of Incomings or Outgoings belonging to an Account which is what the original method did. Do I have to use the number_of_incomings method (adapted for Items), or is there a way to do it for free purely using associations?
John, you should be able to do the module thing, as discussed before:
D’oh! Thanks Jamis, that makes sense.
Hey guys, great articles so far! I’ve picked up so many great refactoring tips from this site.
Just wanted to make a comment to Silvio, I believe @session is deprecated in the upcoming rails version, and you may want to switch them to just session instead. Same goes for @params to params and @request to request.
Hope that helps!