Things don't need to be so hard to read October 8th, 2007

Application.rb seems to be a repository for tidbits of shameful delight. How about getting the current user and checking if we're logged in? Pretty common stuff, yadda yadda.

def user
  return @_user unless @_user.nil?
  unless cookies['logged_in'].nil? || cookies['logged_in'].empty? || !AppConfig.features['personalization']
    @_user = Personalization::User.find :first, :conditions => ["cookie_id = ?", cookies['logged_in']]
  end
  @_user ||= Personalization::CookieUser.new(self.cookies)
  return @_user
end

def logged_in?
  return false unless AppConfig.features['personalization']
  cookies.include? "logged_in" and cookies["logged_in"] and user.is_a? Personalization::User
end

This really doesn't need to be nearly so verbose.

First off, #user is checking all of the same cookies and config settings that #logged_in? checks.

def logged_in?
  user.is_a? Personalization::User
end

What about the user method with all those returns and assignments and conditions?

def user
  @_user ||= find_personalization_user || Personalization::CookieUser.new(self.cookies)
end

private

def find_personalization_user
  if AppConfig.features['personalization'] && !cookies['logged_in'].blank?
    Personalization::User.find :first, :conditions => ["cookie_id = ?", cookies['logged_in']]
  end
end

And we end up with methods that are readable and understandable.

0 comments »

Mixins Don't Replace Inheritance October 4th, 2007

Our application lets you search for businesses based on criteria including name and phone number. Each search could follow a different flow. If a name search returns nothing, check for typos in the name. On a phone search, give up.

This calls for controller inheritance. From a SearchController class, create NameSearchController, PhoneSearchController, etc.

However, Ruby is flexible you can modify an existing class at any point in your application. They can be "reopened." They are prone to abuse.

So instead, we have one giant Search controller. The extracted functionality was moved into modules, which were dynamically added to the main class, or "mixed-in."

Here's a simplified example.

# In name_search.rb
class SearchController < ApplicationController
  module NameSearch
    module Actions
       #name search Specific actions
    end
  end
end
# In search_controller.rb
require File.dirname(__FILE__) + '/search_controller/name_search'
class SearchController < ApplicationController
  include NameSearch::Actions, NameSearch::Reflection
  # common search actions
end

What could possibly go wrong?

You live a dangerous life when you're too clever for inheritance. Watch your method names. Every Ruby developer has accidentally defined a method twice, started editing the first definition, and pulled out their hair as their code changes mysteriously have no effect. Imagine the danger splitting things across files.

Now ask: "How do I know what search I'm performing?" After all, in your layout you'll need to know which navigation tab to highlight.

In the mixin setup:

module Reflection
  def name_search?
    NameSearch::Actions.action_methods.include? params[:action]
  end
end

With a subclass:

def name_search?
   self.kind_of? NameSearchController
end

helper_method :name_search?

Mixins are perfect for multiple inheritance. You just don't need that here.

0 comments »

Naming Your Tests October 3rd, 2007

A common mistake is creating giant tests that test unrelated things. You want lots of small tests that target specific functionality. There are reasons beyond aesthetics and philosophy. In Test::Unit, when one assertion fails, the test haults. If it's the first assertion, you have no idea how broken things are.

Your tests should be short enough to fit its purpose in its name. The finer point in that sentence is "purpose."

What does this test?

def test_tile?
  listing = BusinessListing.new
  listing.tier = 10
  assert listing.tile?
end

How about this?

def test_should_detect_tile_tier_as_tile
  listing = BusinessListing.new
  listing.tier = 10
  assert listing.tile?
end

Starting every test with "test_should," you're encouraged to test for behavior. If this style excites you, check out rspec.

0 comments »

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 »

dry? September 7th, 2007

<% if params[:auto_play] %>
    <%= render :partial => 'more_info_listing_box_video', 
                :locals => {:listing => @listing, :auto_play => true} %>
<% else %>
    <%= render :partial => 'more_info_listing_box_video', 
                :locals => {:listing => @listing, :auto_play => false} %>
<% end %>

Why not...

<%= render :partial => 'more_info_listing_box_video',
            :locals => {:listing => @listing, 
            :auto_play =>(!params[:auto_play].nil? && params[:auto_play] == true)} %>

or something along those lines?

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

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

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