# Refactor this Ruby code for me, please

I always seem to struggle refactoring my code. I can look at any other code and know exactly what is going on, but when it comes to cleaning up my code, I get writers block.

The following code works, but I know if can be done much nicer, and with few lines of code. I just wanted to wrap a memcache client to use one server for testing and another for dev/prod or just fetch the value if it cannot connect to the server.

Any tips on refactoring, etc. are much appreciated.

require 'dalli'
class Cache
def self.fetch(key, ttl, &block)
if memcache
memcache.fetch(key, ttl, &block)
else
block.call
end
end

def self.memcache
begin
if(ENV['RACK_ENV'] == :production or ENV['RACK_ENV'] == :development)
@memcache ||= Dalli::Client.new('cache.amazonaws.com:11211')
else
@memcache ||= Dalli::Client.new('localhost:11211')
end
rescue Exception => e
false
end
end
end

-

This DRY's the code in self.memcache and simply uses a ternary operator for self.fetch.

require 'dalli'
class Cache
def self.fetch(key, ttl, &block)
memcache ? memcache.fetch(key, ttl, &block) : block.call
end

def self.memcache
@memcache ||= Dalli::Client.new((ENV['RACK_ENV'] == :production or ENV['RACK_ENV'] == :development) ?
'cache.amazonaws.com:11211' :
'localhost:11211')
rescue Exception
false
end
end

-
The begin and end keywords you get for free inside the block that is a method definition. –  vgoff Dec 24 '12 at 23:35
I'd change some indentation and blank lines, but basically this is it. One important thing though: dont' write rescue Exception, always rescue StandardError. –  tokland Dec 26 '12 at 19:56
Generally true, tokland, but Exception was what was given in the original question, so rather than assume they are rescuing the wrong thing, I stuck with it. It was not clear what they intended to rescue, though that is likely overreaching. I am sure it wasn't a comment to me, though, I am just saying. The indentation is only there to show a new user what the important part of the above line is, which is to indicate the two options below would be resulting in that parenthesis level. I tried to not affect behavior, only what they asked for. @gp, you want to answer about Exception choice? –  vgoff Dec 26 '12 at 22:04

I'd write it like this, if I really want to have fetch as a class method. If that is not mandatory, I would make fetch the only public instance method, and the rest private instance methods.

 require 'dalli'

class Cache
def self.fetch(key, ttl, &block)
memcache ? memcache.fetch(key, ttl, &block) : block.call
end

def self.memcache
@memcache ||= new_client
end

def self.new_client
begin
Dalli::Client.new(memcache_host)
rescue StandardError
false
end
end

def self.memcache_host
(production? || development?) ? 'cache.amazonaws.com:11211' : 'localhost:11211'
end

def self.production?
environment == :production
end

def self.development?
environment == :development
end

def self.environment
ENV['RACK_ENV']
end
end

-
I like it. Still the begin and end are implicit by virtue of being in a block, this block being the method definition. –  vgoff Dec 30 '12 at 8:26
I just don't like the un-indented rescue. Same as the indented or un-indented private statement, some like it one way, others like the other :) –  David Österreicher Dec 30 '12 at 13:26