The Rails Way: Tracks: Part 1

Tracks: Part 1

Posted by koz Wednesday, November 15, 2006 08:24:00 GMT

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.

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.

123	class Project < ActiveRecord::Base extend NamePartFinderend

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.

# 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.

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.

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’.

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.

Sections: The Rails Way
Tags: rails tracks
Meta: permalink

Comments

Leave a response

Pat Maddox – November 15, 2006 @ 09:27 AM

I’m glad I stayed up a bit later tonight…an earlier post from you said that you’d have an article up tomorrow, but I got this surprise right before goin to sleep.

I agree with everything you guys said basically. In the first case I wouldn’t even extract that method into a module – just stick it right in the model. If you find that more classes need that functionality, then extract it.

Looking forward to this series.
Sean Cribbs – November 15, 2006 @ 02:35 PM

Minor nitpick—:dependent => true is deprecated in favor of :dependent => :destroy or one of its brethren.

Other than that, great article!
Jamis – November 15, 2006 @ 03:18 PM

Right you are, Sean. Thanks for pointing it out. I’ve corrected the article to use :delete_all.
Luke Melia – November 15, 2006 @ 03:48 PM

Great start, guys. It’s a real treat to have code I wrote reviewed like this. I’ll incorporate these suggestions back into Tracks post haste.

Thanks, again. I’m looking forward to the other pieces!
Zack Chandler – November 15, 2006 @ 04:07 PM

The Rails way should be a very interesting series. I really think the rails community will benefit from seeing the way experts tackle/refactor a problem. Looking forward to the coming series. Thanks.
Ryan B. – November 15, 2006 @ 05:40 PM

Good write up! However, I found it a little hard to follow. I think some background information on what the two acts_as_* do would help a lot. I didn’t understand what they did until halfway through the code examples, so I needed to reread it.

Thanks for reminding us about the dangers of premature abstraction. Definitely looking forward to the next article.
Andrew – November 15, 2006 @ 05:51 PM

I agree with Ryan: it would be a helpful if you could more directly compare the code you’re replacing. It’s not 100% clear whether your code samples here are your rewritten code or excerpts from the original. Perhaps just pointing to line numbers in the original that you’re changing would also help?

Also, this series seems to presume a very advanced level of Rails knowledge, how to use modules, composition, etc. Maybe you could at least provide references to those topics when they come up?
Jamis – November 15, 2006 @ 06:14 PM

Andrew, I think this comes of people trying to learn Rails without learning Ruby, which is not a very good strategy. It’s like learning how to build a house without learning what the tools are for. Sure, we all need to start somewhere, but the very first order of business, after you feel comfortable with the fundamentals of Rails, is to hop on over to the Pragmatic Programmers and pick up a copy of Programming Ruby. Your productivity will blossom, I guarantee it.
The Geek – November 15, 2006 @ 07:32 PM

Great stuff.
Brian Ketelsen – November 15, 2006 @ 07:39 PM

I’m really excited about this series and wanted to voice my appreciation. I’m hoping to get rid of my .Net/Java bad habits and I know that the work I did in the very early days of Rails has shaped my thinking about my work now. Hopefully this series will help to show the “new” way to do things too. Pointing out deprecated functionality and newer helpers would help me tremendously. - OH and BRAVO to the Tracks developer for his contribution both to the community with Tracks which I love and to our education as well!
John Topley – November 15, 2006 @ 08:44 PM

Jamis,

Out of interest, how long had you been a Ruby programmer before you got involved with Rails?
Jamis – November 15, 2006 @ 08:53 PM

John, I have been working with Ruby since about 2001, and I started doing Rails work at the end of 2004, so my experience is opposite that to many people coming to Rails. There is no doubt that Rails is a great way to be introduced to Ruby. I really think people are doing themselves a disservice by not embracing Ruby as enthusiastically as Rails, since it is only by understanding Ruby that you can really begin to do the “cool” stuff in Rails. :)
Tim Connor – November 15, 2006 @ 09:21 PM

Learning ruby aside, Jamis, that still leaves Andrew’s first point (which is also Ryan’s point). By explaining what is wrong, but then showing the proper way to do it in your code blocks, you open the door for some confusion. Maybe explanation of problem, original code excerpt, explanation of right way, then fixed code?

I’m not really sure what the best way to present it is, but as it stands there can be a little confusion about what you are presenting with the code blocks, and I don’t think that has anything to do with ruby knowledge. Just a tad bit more context (ideally an excerpt of the code itself) on the original could help a ton.

Of course, don’t take this has anything more than a suggestion – overall I think this is a great thing for all of us plebes to learn more from the Rails gods.
Tim Connor – November 15, 2006 @ 09:22 PM

Of course, I suppose that is what those links to the original are for, eh? Oops :/
Jamis – November 15, 2006 @ 09:27 PM

Ryan, Andrew, and Tim: Koz and I both agree that we need to better at introducing the problem we are discussing. Koz edited the article a bit not long ago to better describe the purpose of the two acts, and we’ll keep this in mind for future articles. Thanks for your feedback! It is a appreciated.
Michael Koziarski – November 15, 2006 @ 10:07 PM

Thanks for the tips so far guys, I’ve added a quick description of what the modules do, and we’ll be sure to make certain that future articles provide a little more intro.
Michael Koziarski – November 15, 2006 @ 10:09 PM

Tim,

We linked to the originals, but even that needs a little more explanation.
Tim Connor – November 16, 2006 @ 02:22 AM

I think once there are a couple articles under everyones belts any conventions you go with should become fairly obvious.
Tim Connor – November 16, 2006 @ 02:25 AM

That is until they implement your suggestions and the link to the HEAD revision changes in some way.
zerohalo – November 16, 2006 @ 04:03 AM

Thanks, Jamis and Koz – great start to the series, and I’m looking forward to more. As a newcomer to Ruby/Rails (trying to study up what I can in my spare time) it takes some re-reading to grasp it, but it’s good “stretching”. I’m afraid I didn’t understand the last paragraph of the class about changing the belongs_to associations, but anyway…

If I understand correctly, the reason for making ‘acts_as_todo_container’ a separate module in the first place is so that it can then be mixed in with both the Project and the Context models? (Your code only shows it being mixed in with the Project model, but I’m assuming that in would be mixed in with Context in the same manner.) Otherwise, it would be better just to have those todo methods be part of the Project model in the project model file rather than a separate module, right?
Luke Melia – November 16, 2006 @ 04:56 AM

That is until they implement your suggestions and the link to the HEAD revision changes in some way.

That’s going to start happening in a little while I created a subversion tag you can use:

guest:guest@www.rousette.org.uk/svn/tracks-repos/tags/rails-way-review/

This is probably a good idea for future reviews, too.
fallenrogue – November 16, 2006 @ 04:58 AM

Bravo! I was just battling this same point today. Argument well made. Keep it up!
Jon Maddox – November 16, 2006 @ 05:37 AM

Great start guys. Maybe before and after code could be determined by different styles. Maybe the line number backgrounds could be different. Something, that sets them apart.
Jakob S – November 16, 2006 @ 08:37 AM

This is great, guys, keep it up. This is definitely going in as subscription in my already burdened newsreader.
Matte – November 16, 2006 @ 11:21 AM

The code is brilliant! Keep up the good work. But I have a question:

In the TodoList module in lines 14 e 23 you use 3 parameters in :conditions, but there are only two conditions: “todos.type = ? and todos.done = ?”. It’s an error or something that I’ve not understood?

Thanks
Shalev – November 16, 2006 @ 02:21 PM

Heh, yeah, I was about to ask that. It seems there’s an extraneous “id” in there?
Shalev – November 16, 2006 @ 02:58 PM

I created a blog/post to help those with limited Ruby/Rails experience understand exactly what is going on here. The post covers the very basics as well as showing before and after code. There are also links to the revision of the svn repository that corresponds to this article (it’s been updated since then so the article no longer deals with the HEAD).
Shalev – November 16, 2006 @ 03:05 PM

And of course forgot to link to it: artisticcoding.blogspot.com/2006/11/rails-path-tracks-part-1.html

Sorry for the multiple posts.
Jamis – November 16, 2006 @ 03:41 PM

Matte, Shalev, good catch! You’re keeping us honest over here. I’ve fixed the article to remove the extraneous id. I also changed the links to point to the branch that Luke so generously provided.

Shalev, awesome post. Thanks for the breakdown! That’s a great supplement to the material here.
Tim Connor – November 16, 2006 @ 05:12 PM

Of course, mightn’ it be wise to grab a local copy, as a habit, to prevent dependencies on other peoples SCM systems?
bsag – November 17, 2006 @ 04:43 PM

These are incredibly useful comments – it’s like a masterclass!

Just one tiny thing: on this article and in part 2, the link to the Tracks project site is wrong: it should be www.rousette.org.uk/projects/. The one you posted doesn’t actually exist, but resolves to my blog.

Thanks!
Jamis – November 17, 2006 @ 05:15 PM

bsag, sorry about that, I’ve corrected the links in both articles. Glad you’re finding these posts useful!

Tracks: Part 1

Comments

About

Series

SignOut!

More Idiomatic Ruby

AssetsGraphed

Importing Files

Idiomatic Ruby

Tracks

Submissions

Sponsors

Syndicate