the blog of shame

just for the fun of it October 25th, 2007

if params[:auto_login].nil?
  cookies["logged_in"] = cookies["logged_in"]
else
  ... stuff that actually does something
end

It seems like we've entered some sort of lightning round. Some things just don't need exposition. Just deletion.

0 comments »

Introducing "or" October 24th, 2007

Someone, please explain:

if (search_request.radius.blank? || search_request.radius.blank?)

1 comments »

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?

4 comments »

Ben Doesn't Read (was: "Cherish Your Routes") October 11th, 2007

Check out the new punchline to this article!

There is no correlation to your application URLs and the location of files in your Rails app. For example, you can take a request for "/losangeles.html" and run the "SouthernCalifornia#index" action. This flexibility comes from Rails' "Routing."

When you hit a URL in Rails, the URL gets run through a series of regular expressions to figure out the action. You define those regexes in your routes.

For performance (and maintainability), keep your routes lean. Here's some low hanging fruit:

map.with_options(:controller => 'about') do |about_map|
  about_map.connect 'about', :action => 'about'
  about_map.connect 'about/:action'
end

map.about 'about/:action', :controller => 'about', :action => 'about'

Plus, we avoid using with_options.

Of course you should test your routes before refactoring. Here's a helper:

def assert_route(url, parameters)
    result = ActionController::Routing::Routes.recognize_path(url)
    parameters.each do |k,v|
      assert_equal v, result[k], "Route recognition fails for #{url}: #{result[k]} => #{v}"
    end
  end

And the test:

def test_should_recognize_about_routes
  assert_route '/about', :controller => 'about', :action => 'about'
  assert_route '/about/madeupaction', :controller => 'about', :action => 'madeupaction'
end

The New Ending

Turns out, I just recreated assert_generates

2 comments »

The Quality of Outsourcing October 10th, 2007

We can thank Eric Mill for forwarding this code. Models have been changed to protect the innocent.

<% if Car.find(@driver.car_id).car != nil %>

Model, VIEW, Controller

You NEVER put finders in your view. I know exceptions, but explaining them will be enough justification for some idiot. Look, you put a finder in your view, you're dead to me.

Did they even watch the screencast?

It's remarkable they don't know ActiveRecord relationship macros. That feature converts people to Rails. The column even follows naming conventions! All it takes:

class Driver
  has_one :car
end

Then you've got:

<% if @driver.car.car != nil %>

Which only leaves the column name. Wouldn't 'name' be more descriptive?

Truth isn't hard

Last, the coder doesn't know the Ruby definition of truth. Anything except false or nil is true. That leaves us with:

<% if @car.name %>

Which doesn't cover an empty string. You really want:

<% unless @car.name.blank? %>

The moral: don't outsource your code overseas.

0 comments »

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 »

Don't Assign Variables in Templates October 5th, 2007

You have an array whose size is unknown. Render all elements less than or equal to 29.

The ugly version:

<%- upper_limit = refinements.length >= 29 ? 29 : refinements.length -%>
<%- 0.upto upper_limit-1 do |index| -%>
  <%- refinement = refinements[index] -%>
  <%= refinement_formatting refinement -%>
<%- end -%>

If you're assigning variables inside ERB templates, you're probably doing something wrong.

Here's the clean version:

<%- refinements[0..28].to_a.each do |refinement| -%>
  <%= refinement_formatting refinement -%>
<%- end %>

Ruby won't complain if you call a range outsize the array's size. It will return a nil, hence the call to "to_a."

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 »

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 -%>

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:

Prints the number 1 to 100
When it reaches a number divisible by 3, print "fizz"
When it reaches a number divisible by 5, print "buzz"
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