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.

0 comments »

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.