relax with your hacks

Jared Carroll

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.

About thoughtbot

We've been helping engineering teams deliver exceptional products for over 20 years. Our designers, developers, and product managers work closely with teams to solve your toughest software challenges through collaborative design and development. Learn more about us.