AssetsGraphed: Part 1
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 |
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.
In response to Koz’s addition, do you need to add the “class” attribute to the has_many call? It seems as though otherwise, Rails will raise an error because it can’t find models for IncomingItem and OutgoingItem.
Maybe this inferring has been improved in Rails 1.2, though; I’m still using 1.1.
Adam, there is no longer any IncomingItem or OutgoingItem. Both of those have been consolidated into a single class: Item. The point of this article as that you don’t need both classes—they only differ based on the sign of the amount, so you can simply use SQL to differentiate the two collections.
Sorry for the confusion, Jamis. My comment was in response to Koz’s addition near the end of the article. He had omitted the ”:class => Item” attribute from the has_many method and I wondered whether that was correct or not.
I realize I mislabled the models to use, but it seems as though it has now been fixed.
The separation of incoming and outgoing items (via
base_amount) should be handled by theItemmodel, IMHO. One clean way of doing that is adding two class methods toItem;find_incomingandfind_outgoing.Assetwill then only have to know that ithas_many :items. ActiveRecord will magically build a correct query if we doasset.items.find_incoming.Let me know if I’ve missed something. I wouldn’t mind a preview button either. :)
Good observation, Christoffer. That’s a really great use of the class-method delegation feature of Rails, too. One issue, though, is memoization, which is part of the reason for putting the methods on the Asset class.
Also, an argument to counter your counterargument (lol) can be made for putting the separation on Assets, since the application treats them conceptually as two separate collections, and collections (in Rails) are defined on the container, via associations.
It’s interesting seeing the subtle differences between Cocoa and Rails. In Cocoa, the do…end block on the has_many association would be never fly (stylistically) because “asset.items.incoming” implies that you can also do “items.incoming”. In other words, the keypaths are assumed to work in any segment, no just in certain combinations.
(I’m not sure if they’re called keypaths in Ruby, but you get the idea :)
Slightly OT, but in regard to the controller comment, Jamis, are you a fan of the RESTful interface ?
I have a similar issue to this, two models that are very close to each other, they are however slightly more differentiated than the ones above. (They basically share about half of their columns/validations/methods).
Would it make sense to create an intermediate class that they both inherit from, instead of ActiveRecord::Base? Or would it be better to include a module in each, with the shared code in the module?
A related question – they also share some attributes that are effectively the same, but named differently (ie. event_date and booking_date). I’ve thought about solving them by having both the setters and getters simply aliases to common methods. Would that make sense?
Thanks!
I really prefer the first approach ( the anonymous one ) because IMHO it self_explain the model structure tree ( item -> (incoming,outgoing)) better.
And… isn’t possible to handle this incoming/outgoing item by extending a base item model ? ( Inheritance )
Sandro.
There are a few more ways you can implement @asset.items.incoming. Perhaps, you want the ability to specify additional find options? Or, maybe you want those methods available in the Item class as well?
I have them in this pastie.
This is an excellent refactor and kind of obvious now that it’s laid out before me! Not surprisingly it’s mentioned in the Martin Fowler book – Pull Up Field.
I think it will get slightly complicated when I come to migrate the existing data to the new structure as I have to preserve the relationships to rows in the balances table.
Sean: I’m very much a fan of RESTful controllers. Rails 1.2 makes it easy enough that there is very little reason not to go RESTful for new apps. The ease of development that follows the conventions make it so nice.
Cameron: if the two models share enough schema, it might be worth throwing them all in the same table and using Single Table Inheritance to implement them. (“enough schema” is something of an art, but I’d say if more than half the columns are identical, you probably ought to try it.) If they only share a few columns, extracting common functionality to a module is a good approach.
rick, thanks for taking the refactoring further! My only concern with your changes is that they make it next to impossible to memoize the result, meaning each time you request that method, you’re doing another query to the database.
I don’t like putting methods on the association proxy like that, it breaks encapsulation. Instead I think you should have
Why should client classes have to know about the existence of the entire items list just to get a subset of them? You get the exact same benefits, but without breaking encapsulation.
You can do Rick’s refactoring to this as well if you want.
I suppose we’ll have to agree to disagree, Pat. I don’t see any violation of encapsulation in the way I’ve described—in fact, I find it nicer in many ways than further multiplying the methods within the Asset namespace. But, I also acknowledge that it is a matter of esthetics, to some extent, so each should choose what suits them best.
I’m not going to convince you of anything, Jamis :) but I do want to provide for others a more concrete reason why I think your way breaks encapsulation.
Let’s say down the road it becomes apparent that we do need separate classes for incoming and outgoing items. As we’re building the class, we notice that a lot of methods are checking the sign of the amount before performing some behavior. We replace conditional with polymorphism (Fowler, Refactoring) to create our two classes.
Now any clients of our Asset class will break. They’re coupled to our proxy implementation, rather than simply being aware of the general concept of incoming and outgoing items.
I’m perfectly content with agreeing to disagree. I do think it’s clear though that the proxy method breaks encapsulation. It prevents you from performing at least one simple, useful refactoring.
I definitely see your point about the memoization with Rick’s refactor, but it seems ashame to let that stop going with that route. Perhaps with a combination of both, there isnt too much duplication (with some help from your later post, Jamis) and you get the best of both worlds.
I’ve also included a pastie to represent the concept: http://pastie.caboo.se/32292
Hopefully the syntax is all correct, just threw it together quickly to get the idea down.
This is a great blog! Especially for people coming from other languages to ruby – how to turn C on Rails to Ruby on Rails :)
I’ll try and submit some of my code in need of refactoring, but in the mean time keep up the great work!
Pat, excellent points. Thanks for the great discussion, I love it! You have an good point about the coupling, but to be honest, I think that’s what unit tests are for. I think it is dangerous to try and code for future contingencies that are not very likely to happen, like what happens if “it becomes apparent that we do need separate classes for incoming and outgoing items”. Yes, that might happen, but so might a host of other things that we can’t foresee. Better to write it how you like it now, and make sure your tests are solid, so that if you do need to refactor down the road you can do so with confidence.
That’s definitely not to say you should just throw code together pell-mell, but the overriding concern, I think, is to code for maintainability, not extensibility. Where you draw that line, though, is very much up to you.
I’m curious to know, so that’s why I’m re-iterating this, if the implementation I proposed above is poorly thought out or nieve.
Jamis, I think what you provide in your writings here is invaluable. I’m new to ruby, so would like to know if this bit follows the “rails way” or is going too far: link here
nkryptic, I’ve blogged a bit more on this topic on my own site, here. In the comments there I explain a bit more about why I prefer not to put methods like that on the association class. By putting the method on the class, you imply that those methods are intended to be called directly, which is rarely the case (since you usually want the calls scoped). If the calls are always to be scoped, then they are internal implementation details, and they ought to be defined within the scope they are needed.
Now, if they are called from multiple scopes, then naturally you’d want to find a way to factor out the duplicate code. But in this case, where the scope is (as far as we know) only Asset, it makes the most sense to keep the definition right there, as close to the Asset as possible.
That’s my take on it, anyway. :)
I’ve just started to implement this refactring. How do I pass an extra parameter to the ItemsController create action using the form_for helper?
Disregard my last question, I’ve worked it out:
<% form_for :item, :url => items_path(:negative => ‘true’) do |f| %>