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 »

Premature Extraction October 2nd, 2007

You should break common HTML fragments into seperate files to keep things DRY. These reusable fragments are called partials.

If you want to render an 'example' partial and pass it the '@variable' variable as 'local_name':

<%= render :partial => 'example', :locals => {:local_name => @variable } %>

Which is pretty "verbose" syntax, but isn't too bad. You can even hide it behind a helper. Today I see this:

<% with_options({:locals => {:listing => listing}}) do |products| %>
  <%= products.render :partial => 'listings/products/services' if listing.services %>
  <%= products.render :partial => 'listings/products/brands' if listing.brands %>
<% end %>

And each partial is basically:

<h3>Brands</h3>
<p>
  <%= with_newlines_as_breaks listing.brands %>
</p>

with_options is a helpful method mixed into #Object by ActiveSupport. It saves space when you need to pass a big hash to several methods. Jamis Buck has a great writeup on it. Here, it's a sign you're being too clever.

This template doesn't need partials. The HTML is never reused. The partials only add obfuscation. When you load the template the first time, first you have to figure out what's going on with the partial. Then, you have to jump to the partial. This is simpler:

<%- if listing.services -%>
  <h3>Services</h3>
  <p>
    <%= with_newlines_as_breaks listing.services %>
  </p>
<%- end -%>
<%- if listing.brands -%>
  <h3>Brands</h3>
  <p>
    <%= with_newlines_as_breaks listing.brands %>
  </p>
<%- end -%>

A good rule of thumb in refactoring is "three strikes and you're out." So if we had another attribute, say "hours," we'd refactor it to:

<%- %w[services brands hours].each do |attribute| -%>
  <%- if listing.send(attribute) -%>
    <h3><%= attribute.capitalize %></h3>
    <p>
        <%= with_newlines_as_breaks listing.send(attribute) %>
    </p>
  <%- end -%>
<%- end -%>

0 comments »

Fizz Buzz October 1st, 2007

It's frustrating to spend hours with a prospective hire only realize he couldn't code to save his life. The only thing worse is finding out after you've hired him.

It's not their fault. Studies show incompetent people don't know they're incompetent. It's up to you to save everyone's time. Use a short test to screen out the fools.

An example is "Fizz Buzz." The instructions are dead simple. Write a program that:

  1. Prints the number 1 to 100
  2. When it reaches a number divisible by 3, print "fizz"
  3. When it reaches a number divisible by 5, print "buzz"
  4. When it reaches a number divisible by both, print "fizz buzz"

Too many candidates can't do this. Even if they can, you can tell how bad they are by the implementation.

Here's a submission ported from Visual Basic to Ruby. It's ported because Ruby is a great common language, and to avoid legal trouble from posting a candidate's submission.

1.upto(100) do |i|
  fizz = false
  buzz = false
  if i % 3 == 0
    fizz = true
  end
  if i % 5 == 0
    buzz = true
  end
  if fizz == false and buzz == false
    puts i
  elsif fizz == true and buzz == false
    puts "#{i} fizz"
  elsif fizz == false and buzz == true
    puts "#{i} buzz"
  elsif fizz == true and buzz == true
    puts "#{i} fizz buzz"
  end
end

Of course he failed before his first line of code. If you're interviewing for a Ruby position, and you're allowed to pick any language, don't pick Visual Basic.

How would I write it?

1.upto(100) do |i|
  output = [i]
  output << 'fizz' if i % 3 == 0
  output << 'buzz' if i % 5 == 0
  puts output.join(' ')
end

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?