Massive refactoring to the internals to simplify

* Move per call configuration out to API/Cache classes thereby reducing knowledge of their internals by APICache class
* Simpler mocked specs for APICache
* Proper specs for the API and Cache classes

lib/api_cache.rb

@@ -4,20 +4,19 @@ class Invalid <  RuntimeError; end
   class CannotFetch < RuntimeError; end
 
   class << self
-    attr_accessor :cache
+    attr_accessor :store
     attr_accessor :api
     attr_accessor :logger
   end
 
   # Initializes the cache
-  def self.start(store = APICache::MemcacheStore, logger = APICache::Logger.new)
-    APICache.logger = logger
-    APICache.cache  = APICache::Cache.new(store)
-    APICache.api    = APICache::API.new
+  def self.start(store = nil, logger = nil)
+    APICache.logger = logger || APICache::Logger.new
+    APICache::Cache.store = (store || APICache::MemcacheStore).new
   end
 
-  # Raises an APICache::NotAvailableError if it can't get a value. You should rescue this
-  # if your application code.
+  # Raises an APICache::NotAvailableError if it can't get a value. You should 
+  # rescue this if your application code.
   # 
   # Optionally call with a block. The value of the block is then used to 
   # set the cache rather than calling the url. Use it for example if you need
@@ -32,6 +31,7 @@ def self.start(store = APICache::MemcacheStore, logger = APICache::Logger.new)
   #   APICache.get \
   #     "http://twitter.com/statuses/user_timeline/6869822.atom",
   #     :cache => 60, :valid => 600
+  # 
   def self.get(key, options = {}, &block)
     options = {
       :cache => 600,    # 10 minutes  After this time fetch new data
@@ -41,21 +41,31 @@ def self.get(key, options = {}, &block)
       :timeout => 5     # 5 seconds   API response timeout
     }.merge(options)
 
-    cache_state = cache.state(key, options[:cache], options[:valid])
+    cache = APICache::Cache.new(key, {
+      :cache => options[:cache], 
+      :valid => options[:valid]
+    })
+
+    api = APICache::API.new(key, {
+      :period => options[:period], 
+      :timeout => options[:timeout]
+    }, &block)
+
+    cache_state = cache.state
 
     if cache_state == :current
-      cache.get(key)
+      cache.get
     else
       begin
-        raise APICache::CannotFetch unless api.queryable?(key, options[:period])
-        value = api.get(key, options[:timeout], &block)
-        cache.set(key, value)
+        raise APICache::CannotFetch unless api.queryable?
+        value = api.get
+        cache.set(value)
         value
       rescue APICache::CannotFetch => e
         APICache.logger.log "Failed to fetch new data from API because " \
           "#{e.class}: #{e.message}"
         if cache_state == :refetch
-          cache.get(key)
+          cache.get
         else
           APICache.logger.log "Data not available in the cache or from API"
           raise APICache::NotAvailableError, e.message

lib/api_cache/api.rb

@@ -4,19 +4,34 @@
 # last queried in case there is a limit to the number that can be made.
 #
 class APICache::API
-  def initialize
-    @query_times = {}
+  @query_times = {}
+  class << self
+    attr_reader :query_times
+  end
+
+  # Takes the following options
+  # 
+  # period:: Maximum frequency to call the API
+  # timeout:: Timeout when calling api (either to the proviced url or 
+  #           excecuting the passed block)
+  # block:: If passed then the block is excecuted instead of HTTP GET against 
+  #         the provided key
+  # 
+  def initialize(key, options, &block)
+    @key, @block = key, block
+    @timeout = options[:timeout]
+    @period = options[:period]
   end
 
-  # Checks whether the API can be queried (i.e. whether retry_time has passed
+  # Checks whether the API can be queried (i.e. whether :period has passed
   # since the last query to the API).
   #
-  # If retry_time is 0 then there is no limit on how frequently queries can
+  # If :period is 0 then there is no limit on how frequently queries can
   # be made to the API.
   #
-  def queryable?(key, retry_time)
-    if @query_times[key]
-      if Time.now - @query_times[key] > retry_time
+  def queryable?
+    if query_times[@key]
+      if Time.now - query_times[@key] > @period
         APICache.logger.log "Queryable: true - retry_time has passed"
         true
       else
@@ -38,15 +53,15 @@ def queryable?(key, retry_time)
   # If the block is unable to fetch the value from the API it should raise
   # APICache::Invalid.
   #
-  def get(key, timeout, &block)
+  def get
     APICache.logger.log "Fetching data from the API"
-    @query_times[key] = Time.now
-    Timeout::timeout(timeout) do
-      if block_given?
+    query_times[@key] = Time.now
+    Timeout::timeout(@timeout) do
+      if @block
         # This should raise APICache::Invalid if it is not correct
-        yield
+        @block.call
       else
-        get_via_http(key, timeout)
+        get_key_via_http
       end
     end
   rescue Timeout::Error, APICache::Invalid => e
@@ -55,8 +70,8 @@ def get(key, timeout, &block)
 
   private
 
-  def get_via_http(key, timeout)
-    response = redirecting_get(key)
+  def get_key_via_http
+    response = redirecting_get(@key)
     case response
     when Net::HTTPSuccess
       # 2xx response code
@@ -70,4 +85,8 @@ def redirecting_get(url)
     r = Net::HTTP.get_response(URI.parse(url))
     r.header['location'] ? redirecting_get(r.header['location']) : r
   end
+
+  def query_times
+    self.class.query_times
+  end
 end

lib/api_cache/cache.rb

@@ -4,10 +4,14 @@
 # cache and delegates storage to the various cache stores.
 #
 class APICache::Cache
-  attr_accessor :store
+  class << self
+    attr_accessor :store
+  end
 
-  def initialize(store)
-    @store = store.send(:new)
+  def initialize(key, options)
+    @key = key
+    @cache = options[:cache]
+    @valid = options[:valid]
   end
 
   # Returns one of the following options depending on the state of the key:
@@ -17,11 +21,11 @@ def initialize(store)
   # * :invalid (data is too old to be useful)
   # * :missing (do data for this key)
   #
-  def state(key, refetch_time, invalid_time)
-    if @store.exists?(encode(key))
-      if !@store.expired?(encode(key), refetch_time)
+  def state
+    if store.exists?(hash)
+      if !store.expired?(hash, @cache)
         :current
-      elsif (invalid_time == :forever) || !@store.expired?(encode(key), invalid_time)
+      elsif (@valid == :forever) || !store.expired?(hash, @valid)
         :refetch
       else
         :invalid
@@ -31,16 +35,22 @@ def state(key, refetch_time, invalid_time)
     end
   end
 
-  def get(key)
-    @store.get(encode(key))
+  def get
+    store.get(hash)
   end
 
-  def set(key, value)
-    @store.set(encode(key), value)
+  def set(value)
+    store.set(hash, value)
     true
   end
-
-  def encode(key)
-    Digest::MD5.hexdigest key
+
+  private
+
+  def hash
+    Digest::MD5.hexdigest @key
+  end
+
+  def store
+    self.class.store
   end
 end

spec/api_cache_spec.rb

@@ -3,70 +3,67 @@
 describe APICache do
   before :each do
     @key = 'random_key'
-    @encoded_key = "1520171c64bfb71a95c97d310eea3492"
-    @data = 'some bit of data'
+    @cache_data = 'data from the cache'
+    @api_data = 'data from the api'
   end
 
   describe "get method" do
-
-    it "should MD5 encode the cache key" do
-      APICache::Cache.new(APICache::MemoryStore).encode(@key).should == @encoded_key
-    end
-    it "should encode the cache key before calling the store" do
-      APICache.cache.should_receive(:state).and_return(:current)
-      APICache.cache.store.should_receive(:get).with(@encoded_key).and_return(@data)
-      APICache.cache.should_receive(:encode).with(@key).and_return(@encoded_key)
-      APICache.get(@key).should != @data
+    before :each do
+      @api = mock(APICache::API, :get => @api_data, :queryable? => true)
+      @cache = mock(APICache::Cache, :get => @cache_data, :set => true)
+
+      APICache::API.stub!(:new).and_return(@api)
+      APICache::Cache.stub!(:new).and_return(@cache)
     end
 
     it "should fetch data from the cache if the state is :current" do
-      APICache.cache.should_receive(:state).and_return(:current)
-      APICache.cache.should_receive(:get).and_return(@data)
+      @cache.stub!(:state).and_return(:current)
 
-      APICache.get(@key).should == @data
+      APICache.get(@key).should == @cache_data
     end
 
-    it "should make new request to API if the state is :refetch" do
-      APICache.cache.should_receive(:state).and_return(:refetch)
-      APICache.api.should_receive(:get).with(@key, 5).and_return(@data)
+    it "should make new request to API if the state is :refetch and store result in cache" do
+      @cache.stub!(:state).and_return(:refetch)
+      @cache.should_receive(:set).with(@api_data)
 
-      APICache.get(@key).should == @data
+      APICache.get(@key).should == @api_data
     end
 
-    it "should return the cached value if the api cannot fetch data and state is :refetch" do
-      APICache.cache.should_receive(:state).and_return(:refetch)
-      APICache.api.should_receive(:get).with(@key, 5).and_raise(APICache::CannotFetch)
-      APICache.cache.should_receive(:get).and_return(@data)
+    it "should return the cached value if the state is :refetch but the api is not accessible" do
+      @cache.stub!(:state).and_return(:refetch)
+      @api.should_receive(:get).with.and_raise(APICache::CannotFetch)
 
-      APICache.get(@key).should == @data
+      APICache.get(@key).should == @cache_data
     end
 
     it "should make new request to API if the state is :invalid" do
-      APICache.cache.should_receive(:state).and_return(:invalid)
-      APICache.api.should_receive(:get).with(@key, 5).and_return(@data)
+      @cache.stub!(:state).and_return(:invalid)
 
-      APICache.get(@key).should == @data
+      APICache.get(@key).should == @api_data
     end
 
-    it "should raise an exception if the api cannot fetch data and state is :invalid" do
-      APICache.cache.should_receive(:state).and_return(:invalid)
-      APICache.api.should_receive(:get).with(@key, 5).and_raise(APICache::CannotFetch)
+    it "should raise NotAvailableError if the api cannot fetch data and state is :invalid" do
+      @cache.stub!(:state).and_return(:invalid)
+      @api.should_receive(:get).with.and_raise(APICache::CannotFetch)
 
-      lambda { APICache.get(@key).should }.should raise_error(APICache::NotAvailableError)
+      lambda {
+        APICache.get(@key).should
+      }.should raise_error(APICache::NotAvailableError)
     end
 
     it "should make new request to API if the state is :missing" do
-      APICache.cache.should_receive(:state).and_return(:missing)
-      APICache.api.should_receive(:get).with(@key, 5).and_return(@data)
+      @cache.stub!(:state).and_return(:missing)
 
-      APICache.get(@key).should == @data
+      APICache.get(@key).should == @api_data
     end
 
     it "should raise an exception if the api cannot fetch data and state is :missing" do
-      APICache.cache.should_receive(:state).and_return(:missing)
-      APICache.api.should_receive(:get).with(@key, 5).and_raise(APICache::CannotFetch)
+      @cache.stub!(:state).and_return(:missing)
+      @api.should_receive(:get).with.and_raise(APICache::CannotFetch)
 
-      lambda { APICache.get(@key).should }.should raise_error(APICache::NotAvailableError)
+      lambda {
+        APICache.get(@key).should
+      }.should raise_error(APICache::NotAvailableError)
     end
   end
 end

spec/api_spec.rb

@@ -1,10 +1,44 @@
 require File.dirname(__FILE__) + '/spec_helper'
 
 describe APICache::API do
-  it "should handle redirecting get requests" do
-    api = APICache::API.new
-    lambda {
-      api.get('http://froogle.google.com', 5)
-    }.should_not raise_error(APICache::CannotFetch)
+  before :each do
+    @options = {
+      :period => 1,
+      :timeout => 5
+    }
   end
-end
+
+  it "should not be queryable? for :period seconds after a request" do
+    api = APICache::API.new('http://www.google.com/', @options)
+    api.should be_queryable
+    api.get
+    api.should_not be_queryable
+    sleep 1
+    api.should be_queryable
+  end
+
+  describe "without a block - key is the url" do
+
+    it "should return body of a http GET against the key" do
+      api = APICache::API.new('http://www.google.com/', @options)
+      api.get.should =~ /Google/
+    end
+
+    it "should handle redirecting get requests" do
+      api = APICache::API.new('http://froogle.google.com/', @options)
+      api.get.should =~ /Google Product Search/
+    end
+
+  end
+
+  describe "with a block" do
+
+    it "should return the return value of the block" do
+      api = APICache::API.new('http://froogle.google.com/', @options) do
+        42
+      end
+      api.get.should == 42
+    end
+
+  end
+end