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

don't mind if I do repeat myself repeat myself August 31st, 2007

if ra[:address] && !ra[:address].nil?
    v = ra[:address]
end

Tests should test the things they're testing August 30th, 2007

def test_center_request
    post :get_centers, {'zip'=>'92507'}
    #it assigned Something    
    assert !assigns.empty?    
    assert_response :success
end

Conversations with common sense:

Common Sense: Um... about that test.

Jacob: What? It's a test! It tests things.

CS: Yeah... What does it test?

J: You know. That the page is, good?

CS: ...

J: I mean we assign something to some instance variable, and the page doesn't error out. That's a good sign, right?

CS: ...

Trilogy of Errors August 30th, 2007

def formatted_value(value)
    return value.split("\n").join("&lt;BR>") if !value.nil?
    return value
end

This method:

  1. Has a vague name.
  2. Is overly complicated.
  3. Generates invalid XML.

Refactored, you get:

def with_newlines_as_break_tags(value)
    value.to_s.gsub("\n", "&lt;br />") if value
end

Brittle URLs August 30th, 2007

What's wrong with this?

http://example.com/info-<%= @listing.source %>/<%= @rating.id %>

Life is easier when you spend 5 minutes learning link_to and rails routing.