SPELUNK'D!?! August 8th, 2008

SPELUNK'D!?! is a term our coworker Jeremie invented. It is the discovery of terrible code whilst navigating the deep bowels of a codebase.

This is one of the finest examples I've ever found.

def parse_query_array_to_hash(strings,key_prefix)
  return {} if !strings || strings.length == 0
  out = {}
  count = 0
  for string in strings
    count += 1
    string = "#{key_prefix}:#{CGI.unescape(string)}"
    k, v = CGI.unescape(string).split(":")
    if out[k]
      if !out[k].is_a? Array  #we hit another one with the same key
        out[k] = [out[k]] # switch it to an array
      end
      out[k] << v #CGI.escape(v)  # append the new one
    else
      out[k] = v #CGI.escape(v)
    end
  end
  out
end

WAIT! What the hell are these two lines doing?

string = "#{key_prefix}:#{CGI.unescape(string)}"
k, v = CGI.unescape(string).split(":")

That's got to be one of the most useless things I've seen in a while. Obviously there are more wrong things, but I can't go on after that.

0 comments »

When functions attack! November 26th, 2007

I was given the unfortunate task of modifying some of our mapping behavior. I was rummaging through the javascript assets when I discovered this.

function populate_directions(f,d,e,b,j,k,l,q,i,z,s,u,n,m){

Good God! I totally understand what all of those variables do. Especially when I look at the function comments.

// The following code Overrides VE5.0 directions.

Okay. Maybe not.

Maybe I should take a glance through the method and see if I can gain any insight from context.

Four lines in...

  try{
     if(d==null||b==null||j==null||k==null||l==null||q==null||i==null||s==null||u==null||n==null||m==null)
         throw new VEException("VEDirectionsManager:GetDrivingDirections","err_noroute",L_noroute_text);

All right. I'll admit it. I still have no cluue what's going on. At all.

In addition. Later we assign to v, x, p, o, h, w, r, c, t, a, g and y.

Wait... That's every single letter in the DAMNED ALPHABET! That's right, a through z! What will we do now?

var B=new VERoute(v,x,h)

AUUUUUUUUUUUUUUUUUUURHRGHGHGHGGH!

Who needs models? (part 2) November 14th, 2007

Earlier I left out what lived in the get_captcha_image method. I only did so because it merited a shaming of its own.

drumroll

def get_captcha_image
    srand(Time.now.to_i)
    captcha_img=rand(9999).to_s
    case captcha_img.length
      when 1
        captcha_img = "000" + captcha_img
      when 2
        captcha_img = "00" + captcha_img
      when 3
        captcha_img = "0" + captcha_img
    end
    "/images/captcha/cp" + captcha_img[0].chr + "/cp" + captcha_img[1].chr + 
      "/captcha" + captcha_img[0].chr + captcha_img[1].chr + captcha_img[2].chr + captcha_img[3].chr + ".jpg"
  end
case captcha_img.length
      when 1
        captcha_img = "000" + captcha_img
      when 2
        captcha_img = "00" + captcha_img
      when 3
        captcha_img = "0" + captcha_img
    end

I looked at that, and then did something drastic. I looked at the docs.

It turns out that this "fancy thingamajig" does the exact same thing. captcha_img.rjust(4, "0")

Then there's this gem. "/captcha" + captcha_img[0].chr + captcha_img[1].chr + captcha_img[2].chr + captcha_img[3].chr + ".jpg"

Reading that out loud I'd say: "Print /captca, print the first char of the string, print the second char of the string, print the third.... wait a second! I'm printing the whole damn string one char at a time!"

When simplified, the method ended up looking more like this.

def get_captcha_image
  srand(Time.now.to_i)
  captcha_img=rand(10000).to_s.rjust(4, "0")
  "/images/captcha/cp#{captcha_img[0].chr}/cp#{captcha_img[1].chr}/captcha#{captcha_img}.jpg"
end

What? October 15th, 2007

This was found in the homepage of a site which shall (thankfully) remain anonymous.

<% if @ids and @ids.size > 0 %>
<% @j = 0 %>
<% for i in @ids %>
<% @arr[@j] = i.id %>
<% @j = @j + 1 %>
<% end %>

Usually we try to post something constructive after this point, but .... what? It could have been in a helper method. It could have variables with useful names. It could have used eachwithindex. I don't really care!

Just. What?

When a method becomes an epic journey September 18th, 2007

It's going to be hard to read it, because the the blog of shame's column can't handle the might of this snippet. Trust me when I say that there are things that are wrong here.

Snippet might be the wrong word. It makes it sound quaint or small...

def get_recycle_centers
        begin
        zip_code = h params[:zip]
        #if it has the -dddd then get rid of the last part of that zip
        if (zip_code =~ /^\d{5}-\d{4}$/) == 0
          zip_code = zip_code[0...-5]  
        end

        recycle_centers = Zip.find_by_zip(zip_code).recycling_affiliates

        if recycle_centers.size == 0
           render :text => "<div style='margin-left:10px;' id='no_zip'><h4 style='padding-top:15px;'>There Are No Affiliates Near ZIP Code "+ zip_code+"</h4>
                            For recycling information and resources near you, you can search for 
                            <a href='/"+zip_code+"/Recycling-Centers?t=Recycling+Centers'>Recycling Centers</a> or 
                            <a href='/"+zip_code+"/Recycling-Equipment-Services?t=Recycling+Equipment+%26+Services'>Recycling Equipment & Services.
                            </a>
                            <div style='padding-bottom:100px;'></div>" and return  
        end
        @centers = recycle_centers
        render :partial => 'recycle_centers'        
      rescue
          render :text => "<div style='margin-left:10px;' id='no_zip'><h4 style='padding-top:15px;'>We did not recognize the ZIP code you entered. Please check it and try again.</h4></div><div style='padding-bottom:100px;'></div>"       
      end
    end

To summarize:

  1. That zip code regex seems like a bit of overkill when we only care about the first five chars
  2. We're using a begin/rescue block as a logical control
  3. render :text => WTFAWHOLEBUNCHOFSTUFF
  4. Plus, the HTML in the render :text is missing one of the closing div tags
  5. Building links to specific actions by hand
  6. Assigning the temp variable recycle_centers, checking its size, and then assigning it to @centers

Let me know if I forgot anything.

How about this?

def get_recycle_centers    
    zip_code = h params[:zip]
    zip_code = zip_code[0..4] unless zip_code.nil? #just grab the five digit zip code
    if @zip_centroid = ZipCentroid.find_by_zip(zip_code)
      @centers = @zip_centroid.recycling_affiliates
      render :partial => 'recycling_centers'
    else
      render :text => "<div id='no_zip'><h4>We did not recognize the ZIP code you entered. Please check it and try again.</h4></div>"
    end
end
  1. No more rescue! We're checking for nil on @zip_centroid like a good citizen
  2. Moving the big bulky error message into the view, which really only depends upon @centers being empty or not
  3. The links, which are now in the view, actually use link_to
  4. Reduced extra assignments, renders, ifs and other such items
  5. Got rid of inline styles that were identical in the two original render :text calls

I could probably do more. It's a good start, though.

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.