Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question
add comment

2 Answers 2

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
share|improve this answer
    
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
1  
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
add comment

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
share|improve this answer
    
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
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.