relax with your hacks

Posted by jcarroll

Jan 14

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.

Comments on this post

avi

Jan 14

avi said,

I agree with all those except Symbol#to_proc, which I feel is clean and useful.

sandofsky

Jan 14

sandofsky said,

Symbol#to_proc is so beloved it is standard in Ruby 1.9.

Piers Cawley

Jan 14

Piers Cawley said,

I think you’re kicking a dead horse up hill arguing against Symbol#to_proc there Jared.

And your STATES enum is a state object waiting to get out, once you’ve introduced that, you can look for the code that calls pending?, approved? and submitted? and have them do double dispatch using the state object and get rid of a bunch of conditionals. Result.

Piers Cawley

Jan 14

Piers Cawley said,

And, while I’m about it, that use of with_options is defendable too – what it says is “The fact these three declarations of a relationship with the comment object have the same order is not a coincidence” – something which can only be guessed at on reading the repetitious version of the code.

Where do you stand on using

1
2
3
4


returning(long expression) do |narrowly_scoped_name|
  ...
end

without using the return value? I use it often enough that I’m thinking of implementing something like:


def with(expr, &block)
  yield(expr)
  return
end

With the explicit return making sure that it’ll only really be used in a void context.

Sean

Jan 14

Sean said,

I see both your point and Piers’s point on the use of with_options. Using with_options, it almost looks like a shoulda context.

Floyd

Jan 14

Floyd said,

The two code blocks in your first example are not equivalent. Looks like some logic got lost in all that rewriting.

Tammer Saleh

Jan 14

Tammer Saleh said,

Most developers scanning this are going to be “what? you can use #with_options in a model? i thought that was only for routes.”

I think my main gripe with this advice is that it encourages targeting your code to the least common denominator of programmers. I agree that code can become overly “tricky”, and that should definitely be avoided. I think I just don’t agree where that line is. Any good rails programmer should immediately know that #with_options isn’t just for routes.

Also, you seem to think that being DRY is just a method of saving keystrokes. The main benefit of DRY code is to reduce programmer errors. Take the states example. If a programmer has to add a new state in the moist version, they must also remember to add a new #state? accessor (and any #find_state finders). With the DRY version, it’s just a matter of changing the constant. There are fewer chances for typos, and fewer chances of forgetting part of the process with the DRY version.

Piers Cawley

Jan 14

Piers Cawley said,

Rather than using with_options, another option might be to introduce a new class method to ActiveRecord::Base which can be used to declare variants based on an an existing relationship. Off the top of my head, something like:

1
2
3
4
5
6
7


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

has_variants_of :all_comments do
  comments :conditions => {:approved => true], :dependent => :delete_all
  unapproved_comments :conditions => {:approved => false}
end

should be implementable. It DRYs up the code, and clearly expresses that comments and unapproved_comments are special cases of all_comments.

Roman Gonzalez

Jan 15

Roman Gonzalez said,

Another option for the comments extension, would be using the scope_out plugin 
1
2
3
4


has_many :comments
scope_out :approved_comments, :conditions => { :approved => true }
scope_out :unapproved_comments, :conditions => { :approved => false }
Then in the client you could use: 
1
2
3


Post.find_approved_comments(:all)
Post.find_unapproved_comments(:all)

Anyway this is not about the post per-se. I think there is a limit in the use of hacks, normally I’ll use a lot

1
2
3
4


[:model1, :model2, :model3].each do |association|
  has_many association, { <common parameters> }
end

I think that this is still readable and you avoid much more less typos :-)

Sorry, comments are closed for this article.

© 2000 - 2009 by thoughtbot, inc.
written by a bushel of tiny robots