Welcome to Giant Robots Smashing Into Other Giant Robots — a weblog about development, business, design and technology — written by thoughtbot.

relax with your hacks

Good code has a rhythm. To me code with a good rhythm can be easily read and understood by any developer with a decent amount of experience in the language. What this usually means is avoiding hacks and tricks that you do to impress other developers with your amazing 5ki11z.

Lets take a look at some Rails examples.

From the Rails docs (I added the class..end block)

1
2
3
4
5
6
7
8
9
10


class Post < ActiveRecord::Base

  with_options :order => 'created_at', :class_name => 'Comment' do |post|
    post.has_many :comments, :conditions => ['approved = ?', true], :dependent => :delete_all
    post.has_many :unapproved_comments, :conditions => ['approved = ?', false]
    post.has_many :all_comments
  end

end

Are you kidding me?

Most developers scanning this are going to be “what? you can use #with_options in a model? i thought that was only for routes.” This is such a weak attempt at being DRY and showing off. Nesting the association declarations within the #with_options also puts them syntactically where they’re not expected, hurting readability.

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


class Post < ActiveRecord::Base

  has_many :comments, 
    :conditions => ['approved = ?', true], 
    :order => 'created_at',
    :dependent => :delete_all

  has_many :unapproved_comments, 
    :conditions => ['approved = ?', false],
    :class_name => 'Comment',
    :order => 'created_at'

  has_many :all_comments
    :class_name => 'Comment',
    :order => 'created_at'

end

That is much better. Its clear, explicit and uses constructs any Rails developer would know. I really don’t care that I wrote the same :order parameter 3 times. The associations are much clearer when they’re not nested as well.

Here’s another one I’ve seen

1
2
3
4
5
6
7
8
9


class User < ActiveRecord::Base

  3.times do |each|
    has_one :"car_#{each}",
      :class_name => 'Car'
  end

end

Now this may be just poor modeling, representing the business rule that a user can only have 3 cars as 3 has_one associations instead of 1 has_many association with some validations, but this is also a case of being way too DRY and smart. That code is hideous and takes a few minutes before you actually understand what’s going on. Once again the nesting of the association declarations makes it take longer to realize what’s going on.

1
2
3
4
5
6
7
8
9
10
11


class User < ActiveRecord::Base

  has_one :car_0, 
     :class_name => 'Car'
  has_one :car_1, 
      :class_name => 'Car'
  has_one :car_2, 
      :class_name => 'Car'

end

Much better.

Here’s one more (forget using acts_as_state_machine or any other state machine plugin).

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


class Order < ActiveRecord::Base

  STATES = %w(Pending Approved Submitted)

  STATES.each do |s|
    define_method "#{s}?" do
      state == s.to_s
    end
  end

end

This metaprogramming hack is just ugly. Instead of defining the methods #pending?, #approved? and #submitted?, the author decided to be DRY and smart and use #define_method. Understanding this code takes time and isn’t immediately obvious as to what’s happening.

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


class Order < ActiveRecord::Base

  STATES = %w(Pending Approved Submitted)

  def pending?
    state == 'Pending'
  end

  def approved?
    state == 'Approved'
  end

  def submitted?
    state == 'Submitted'
  end

end

OK so we write a few more lines of code. Who cares? This code is much easier to read and understand quickly. Furthermore the metaprogramming version makes it impossible to ‘grep’ the codebase for the #pending?, #approved? and #submitted? methods because they’re dynamically generated, a big downside to any metaprogramming especially when there’s a lack of documentation.

And of course

1
2


  @users.collect(&:name)

vs.

1
2


  @users.collect {|each| each.name}

Oh I cant stand that Symbol#to_proc hack. Totally useless when you need to send each object in the collection more than 1 message. But people insist on using it and end up creating this mess of inconsistent code by using it when you can and not using it when you can’t. I would much rather have consistent code that always uses the traditional looping style and not Symbol#to_proc.

About this entry

You're reading an entry on GIANT ROBOTS SMASHING INTO OTHER GIANT ROBOTS, the company weblog of thoughtbot, inc.

Author:
Jared Carroll
Published:
January 14th 01:08 AM
Updated:
January 14th 10:05 AM
Sections:
Development

 

thoughtbot is hiring

We are hiring web developers and web designers in both Boston and New York, NY.

What are we up to?

We built Shoulda, an eclectic set of additions to Test::Unit; Paperclip to manage uploaded files without hassle; factory_girl a replacement for Rails fixtures; Jester, a REST/ActiveResource client written in Javascript, and Squirrel, an enhancement for ActiveRecord's find syntax; — amongst some other projects.


Chad (President) and Jon (CTO) co-authored a technical book titled Pro Active Record: Databases with Ruby and Rails, which explores the ins and outs of the ActiveRecord ruby library. You can buy it today at Amazon.com.

About thoughtbot, inc.

We are a small web application development consulting business, with offices in Boston, MA and New York, NY. If you're looking to find a team for your next web development project or your new web application — get in touch.