You're trying too hard August 31st, 2007

I inherited this code on a freelance project, an online school. This method is for finding alerts assigned to users. This code is copied and pasted, unaltered.

def Alert.alerts_for_user_id(user_id)
    # Big ass query alert
    alerts = []
    Alert.find_all(["destination_id = ?", user_id], "id desc").each do |alert|
      alert_object = eval("#{alert.type}.find #{alert.id}")
      #alert_object = alert
      alert_object.destination_username = User.find(alert.destination_id).username
      begin
        alert_object.source_username = User.find(alert.source_id).username
      rescue
      end
      if alert.destination_id == alert.source_id
        alert_object.source_username =  "You"
      end
      alerts << alert_object
    end
    alerts
end

Eval is evil

Let’s start with the first crime against humanity:

eval("#{alert.type}.find #{alert.id}")

If you’re using Single Table Inheritance, the least you could do is:

alert.class.find alert.id

And avoid the performance penalty of eval. But even then, THERE IS STILL NO POINT.

Read the docs, maybe?

This smoking gun:

alert_object.destination_username = User.find(alert.destination_id).username

shows someone doesn’t understand has_many. How’s about:

belongs_to :destination, :class_name => 'User', :foreign_key => 'destination_id'

which gives you:

alert_object.destination.username

Which opens the door to eager loading, to prevent an N + 1 query problem. Repeat for ‘source’.

Refactored…

Here’s the final alternative.

def self.alerts_for_user_id(user_id)
    find(:all, :conditions => ["destination_id = ?", user_id],
        :include => [:source, :destination], :order => "id desc")
end

In closing

The man who wrote that code abandoned the project not because his stupidity was uncovered, but he was offered to be senior developer at another company for “a giant paycheck.”