I’ve recently have the “opportunity” to do some work on the first Rails app I developed professionally, as well as an existing codebase that’s in a similar state.
We’ve all been there. Trying to understand old code, refactoring, adding features and tests. It’s painful. And considering how much Rails has changed in the last 2 years, it’s no wonder that working with apps written for older versions is no fun.
Here’s a few “worst practices” that I’ve encountered in my own and others’ old code:
- Using
url_forinstead of named routes and RESTful routes. Just say no to relying on Rails default, implicit routing. I even delete the default route fromroutes.rb. <% @objects.each do |obj| %>...<% end %>instead of<%= render :partial => 'object', :collection => @objects %>- Lots of
ifblocks inapplication.rhtmlinstead of creating new layouts. - Not using
content_for :whateverto insert code in layouts. - Not following Rails convention, like misnamed controllers:
UserControllerinstead of UsersController - Practically any use of scaffolding (my beef with scaffolding: code generation results in a lot of views you ultimately won’t need, which then get in your way.).
- Not using Rails association methods:
Thingy.find(owner.thingy_id)instead ofowner.thingy - Similarly, not using the built in create and build methods on associations:
Employee.create(:employer => employer, :name => "Bob")instead ofemployeer.employees.create(:name => "Bob") - Security issues. XSS and attributes that should be
attr_protectedfirst among them. - Bad or non-existent tests.
- Deprecated code. It probably wasn’t your fault, but given the pace of change in Rails, working with an old app almost certianly means working with code that’s now deprecated.
Do you have any of your own favorite newb mistakes? Add ‘em in the comments.
Photo by chaim zvi.
My first Rails app has some of the fattest controllers I’ve ever seen.
About two months ago I decided to go all partial-collection on my view loops. Then I promptly created a calendar widget with a year, month, week and day partial, all but one of which contained a single line. I wondered if I was really making the right choice. :)
Most of the code I still play in is un-RESTful, so I’m sadly working in an area I know I should avoid. My first app has big, fat controllers. When things got complex I dropped into SQL, even though I’m not very good at it, because I didn’t quite grock the flexibility of AR.
Unfortunately, that first app I wrote is one I should still be working with. The stinkiness of the code makes working on the app unpleasant enough that I subconsciously avoid it.
meekish—Good one. Fat controllers (and fat views) are definitely an anti-pattern. And I see way to much of it in older code.
I dug through one of my first Rails apps recently, and saw a lot of these. No, all of these. :) I also noticed a few more:
Jon—Not to mention not using the very helpful Rails extensions to Enumerable.
I thought of another one as well: not creating custom associations and methods on associations for commonly used finders. Those can save a lot of code.
If you do something like this:
user.pages.find(:all, :conditions => "status = 'approved'")user.approved_pageswith a custom association oruser.pages.approvedwith a custom method on the association.You should use attr_accessible instead of attr_protected. Why? See this video http://railscasts.com/episodes/26