gistfile1.rb

LOOKUP_CHAIN = [:params, :user, :session, :http, :default]

def self.set_by(inputs)
  locale = nil
  LOOKUP_CHAIN.each do |lookup|
    locale = send("by_#{lookup}", inputs[lookup])
    break if locale
  end
  return locale
end

Not much of an improvement

  def self.set_by(inputs)
    LOOKUP_CHAIN.each do |lookup|
      if locale = send("by_#{lookup}", inputs[lookup])
        return locale
      end
    end
  end

def self.set_by(inputs)
  LOOKUP_CHAIN.collect do |lookup| 
    send("by_#{lookup}", inputs[lookup]) }
  end.detect { |lookup| lookup }
end

@nhessler message dispatch (esp. via send) is pretty expensive, and we don't know what @jcasimir is doing in the code being invoked on each iteration. He could be making calls to the DB or other things on each go-round. My guess is that there's a nontrivial performance loss by going through the whole list, or he wouldn't be asking. :)

Actually, in the inject based one, the ||= can just be an ||

@ernie understood, but until it's an actual bottleneck I tend to go for what works. either way I really like @JoshCreek 's implementation using inject and including the update of just using ||. it makes it pretty and performant.

Didn't have the time to try it and maybe it won't work but just as starting point:

    def self.set_by(inputs)
      LOOKUP_CHAIN.map {|lookup| send("by_#{lookup}", inputs[lookup]) }.
        reduce { |value| value.nil? ? next : value }
    end

or maybe just

  def self.set_by(inputs)
    LOOKUP_CHAIN.map {|lookup| send("by_#{lookup}", inputs[lookup]) }.
      reduce { |value, lookup| value }
  end

@jcasimir how about:

class Test
  LOOKUP_CHAIN = [:params, :user, :session, :http, :default]

  def self.set_by(inputs)
    results = Hash.new {|h, k| h[k] = send("by_#{k}", inputs[k]) if k}
    results[LOOKUP_CHAIN.detect {|k| results[k]}]
  end

  def self.by_user(input)
  end

  def self.by_session(input)
  end

  def self.by_http(input)
    puts "ran this"
    1
  end

  def self.by_params(input)
  end

  def self.by_default(input)
  end
end

puts Test.set_by({})
# => ran this
# => 1

The downside -- the intent isn't as clear with this code, unless you're familiar with Hash block behavior.

Explanation:

The hash set up on the first line of the method runs the block on missing keys, and populates the value with the results of running the code being tested. As such, the second line only evaluates each option once (in the inner block) and detect will return this (working) lookup type, and the outer hash retrieves the stored value. The "if k" is necessary in the block to catch the case where no result is returned by any option, to prevent a NoMethodError on a by_ method. Unnecessary if by_default always returns a result.

A similar method for caching dispatch methods is used in Squeel and ARel.

Not saying it's necessarily better than what you've got (for the reason outlined above) but it is an option. :)

I experimented with method fallback about a year ago here https://gist.github.com/943663
You could use it like this

object.fallback(:params, :user, :session, :http, :default)

with a little tweak to add the "by_" part

ok with the provided test case it's simpler to tri it out and it could be

def self.set_by(inputs)
      LOOKUP_CHAIN.map {|lookup| send("by_#{lookup}", inputs[lookup]) }.detect { |value| value }
end

@gicappa replace the map with detect and it will return nil if none are found.
That looks like the sanest way to write it.

@tyre You can't change the map to detect because then it will return the first value in LOOKUP_CHAIN with a truthy return value, but not the actual result of evaluating the block itself. You'll be getting a symbol from LOOKUP_CHAIN as your final result, versus the actual locale.

Oh yes that makes perfect sense. In that case an explicit return unless the send is nil? ``` ruby { (locale = send(...)) && return locale } ```

…

On Friday, August 3, 2012 at 7:47 AM, Ernie Miller wrote: @tyre You can't change the map to detect because then it will return the first value in LOOKUP_CHAIN with a truthy return value, but not the actual result of evaluating the block itself. You'll be getting a symbol from LOOKUP_CHAIN as your final result, versus the actual locale. --- Reply to this email directly or view it on GitHub: https://gist.github.com/3247167

A test harness of sorts, so that we can verify that our changed versions match the original:

class LocaleChooser


    LOOKUP_CHAIN = [:params, :user, :session, :http, :default]

    def self.set_by(inputs)
        locale = nil
        LOOKUP_CHAIN.each do |lookup|
            locale = send("by_#{lookup}", inputs[lookup])
            break if locale
        end
        return locale
    end


    # define self.by_params(input), self.by_user(input), etc.
    class << self
        returning_lookups = [:user, :http] # edit me for testing

        LOOKUP_CHAIN.each do |lookup|
            define_method("by_#{lookup}") do |input|
                puts "evaluated by_#{lookup}"

                if returning_lookups.include?(lookup)
                    return [lookup, input]
                else
                    return nil
                end
            end
        end
    end
end

p LocaleChooser.set_by({})
puts '---'
p LocaleChooser.set_by(:user => :custom_user) 
puts '---'
p LocaleChooser.set_by(:default => :custom_default)
puts '---'
p LocaleChooser.set_by(:params => :custom_params)

This harness is based on the code in ernie’s comment. The main part of the code, separated with whitespace, is straight from this Gist.

If you run this as shown above (on Ruby 1.9), you get this output:

evaluated by_params
evaluated by_user
[:user, nil]

---
evaluated by_params
evaluated by_user
[:user, :custom_user]

---
evaluated by_params
evaluated by_user
[:user, nil]

---
evaluated by_params
evaluated by_user
[:user, nil]

Change the calls at the bottom and the contents of returning_lookups to test different behaviors.

Here’s a refactoring:

module Enumerable
  def find_mapped
    result = nil
    self.each do |lookup|
      result = yield(lookup)
      break if result
    end
    return result
  end
end

LOOKUP_CHAIN = [:params, :user, :session, :http, :default]

def self.set_by(inputs)
  LOOKUP_CHAIN.find_mapped do |lookup|
    locale = send("by_#{lookup}", inputs[lookup])
  end
end

It produces the same output in the test harness I posted previously.

Oh, and sorry for the 4-space indentation in that previous test-harness code. I forgot to convert from tabs to spaces, and GitHub won’t let me edit that comment.

I made a Gist fork for my refactoring that I just posted.

I just found out that you can make more than one fork of a Gist. So here’s a Gist fork for the test harness I posted earlier.

This Gist was posted alongside a tweet by the author:

Any ideas to refactor this little ruby collection operation? https://gist.github.com/3247167

Thought about this the other day but just got around to trying it this afternoon. I believe #any? stops when it reaches a truthy result, so how about this:

def self.set_by(inputs)
  locale = nil
  LOOKUP_CHAIN.any? { |lookup| locale = send("by_#{lookup}", inputs[lookup]) }
  return locale
end

Passes the test harness.