seen during a code review

rails_class_level_memoization.md

The following code came up during a code review:

def self.account_for(email)
  @account_for ||= {}
  return @account_for[email] if @account_for.include?(email)
  @account_for[email] = Account.find(email)
end

Always one to make a mountain out a molehill ;-) I posted the following comments:

I generally prefer the single-exit-point patterns for doing instance variable memoization:

# use a single method
def foo
  @foo ||= begin
    multiple_lines
    in_order_to_process
    and_create_foo
  end
end
# use two methods
def foo
  @foo ||= compute_foo! # or find_foo! or just foo!
end
# use ActiveSupport::Memoizable
# (keeping in mind that memoized methods get called on freeze
# and freeze gets called on destroy)
def foo
  multiple_lines
  in_order_to_process
  and_create_foo
end
memoize :foo
# in this case I might re-write it as:
def account_for(email)
  @account_for ||= {}
  @account_for[email] ||= Account.find(email)
end
# if I'm in the mood for slightly too-clever code,
# then I can use the Hash default function or Hash#fetch
def account_for(email)
  @account_for = Hash.new do |cache, email|
    cache[email] = Account.find(email)
  end
end

Except for that last one, these are easier to read and less likely to harbor hidden typos or bugs.

But... (everything before "but" is irrelevant)

Keep in mind that class instance variables are an (in-memory) process level cache, and a hacky (albeit simple) one at that. No invalidation, no expiration, no max-memory setting, thread-safety issues, etc. This sort of caching is often given the dirty label of "memory leak" or "bugs that only pop up in esoteric conditions and take forever to debug". ;) I'd feel much better if it were rewritten as something like the following:

# app/models/account.rb
class Account
  def cached_find(email)
    Rails.cache.fetch("account:#{email}") do
      Account.find(email)
    end
  end
  # cache invalidation:
  after_save do |a| Rails.cache.write("account:#{a.email_was}") end
  after_destroy do |a| Rails.cache.delete("account:#{a.email_was}") end
  # preseeding (you should combine invalidation and pre-seeding unless a.email_changed?)
  after_save do |a| Rails.cache.write("account:#{a.email}", a) end
end
# config/initializers/cache.rb
config.cache_store = :memory_store

That's thread-safe, won't bloat memory indefinitely, can be trivially swapped out for a memcached store, etc. Caveat emptor: I haven't recently tested the above code (although I've used code very much like it in the past).

See http://api.rubyonrails.org/classes/ActiveSupport/Cache/Store.html#method-i-fetch on the civilized way to do cache-lookup in rails. :)