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)

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 #withoptions 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 #withoptions also puts them syntactically where they’re not expected, hurting readability.

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

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.

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

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.

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

@users.collect(&:name)

vs.

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