Growing the User abstraction; refactoring

lib/split/helper.rb

@@ -115,16 +115,7 @@ def is_ignored_ip_address?
     end
 
     def active_experiments
-      experiment_pairs = {}
-      ab_user.keys.each do |key|
-        key_without_version = key.split(/\:\d(?!\:)/)[0]
-        Metric.possible_experiments(key_without_version).each do |experiment|
-          if !experiment.has_winner?
-            experiment_pairs[key_without_version] = ab_user[key]
-          end
-        end
-      end
-      return experiment_pairs
+      ab_user.active_experiments
     end
 
     def normalize_metric(metric_descriptor)

lib/split/trial.rb

@@ -49,7 +49,7 @@ def complete!(goals=[], context = nil)
     # method is guaranteed to only run once, and will skip the alternative choosing process if run
     # a second time.
     def choose!(context = nil)
-      @user.cleanup_old_experiments
+      @user.cleanup_old_experiments!
       # Only run the process once
       return alternative if @alternative_choosen
 
@@ -102,22 +102,12 @@ def should_store_alternative?
 
     def cleanup_old_versions
       if @experiment.version > 0
-        keys = @user.keys.select { |k| k.match(Regexp.new(@experiment.name)) }
-        keys_without_experiment(keys).each { |key| @user.delete(key) }
+        @user.cleanup_old_versions!(@experiment)
       end
     end
 
     def exclude_user?
-      @options[:exclude] || @experiment.start_time.nil? || max_experiments_reached?
-    end
-
-    def max_experiments_reached?
-      !Split.configuration.allow_multiple_experiments &&
-          keys_without_experiment(@user.keys).length > 0
-    end
-
-    def keys_without_experiment(keys)
-      keys.reject { |k| k.match(Regexp.new("^#{@experiment.key}(:finished)?$")) }
+      @options[:exclude] || @experiment.start_time.nil? || @user.max_experiments_reached?(@experiment.key)
     end
   end
 end

lib/split/user.rb

@@ -8,7 +8,7 @@ def initialize(context)
       @user = Split::Persistence.adapter.new(context)
     end
 
-    def cleanup_old_experiments
+    def cleanup_old_experiments!
       user.keys.each do |key|
         experiment = ExperimentCatalog.find key_without_version(key)
         if experiment.nil? || experiment.has_winner? || experiment.start_time.nil?
@@ -17,6 +17,34 @@ def cleanup_old_experiments
       end
     end
 
+    def max_experiments_reached?(experiment_key)
+      !Split.configuration.allow_multiple_experiments &&
+        keys_without_experiment(user.keys, experiment_key).length > 0
+    end
+
+    def cleanup_old_versions!(experiment)
+      keys = user.keys.select { |k| k.match(Regexp.new(experiment.name)) }
+      keys_without_experiment(keys, experiment.key).each { |key| user.delete(key) }
+    end
+
+    def active_experiments
+      experiment_pairs = {}
+      user.keys.each do |key|
+        Metric.possible_experiments(key_without_version(key)).each do |experiment|
+          if !experiment.has_winner?
+            experiment_pairs[key_without_version(key)] = user[key]
+          end
+        end
+      end
+      experiment_pairs
+    end
+
+    private
+
+    def keys_without_experiment(keys, experiment_key)
+      keys.reject { |k| k.match(Regexp.new("^#{experiment_key}(:finished)?$")) }
+    end
+
     def key_without_version(key)
       key.split(/\:\d(?!\:)/)[0]
     end

spec/user_spec.rb

@@ -4,9 +4,9 @@
 require 'split/user'
 
 describe Split::User do
-  let(:context) do
-    double(:session => { split: { 'link_color' => 'blue' } })
-  end
+  let(:user_keys) { { 'link_color' => 'blue' } }
+  let(:context) { double(:session => { split:  user_keys }) }
+  let(:experiment) { Split::Experiment.new('link_color') }
 
   before(:each) do
     @subject = described_class.new(context)
@@ -16,26 +16,33 @@
     expect(@subject['link_color']).to eq(@subject.user['link_color'])
   end
 
-  context '#cleanup_old_experiments' do
-    let(:experiment) { Split::Experiment.new('link_color') }
+  context '#cleanup_old_versions!' do
+    let(:user_keys) { { 'link_color:1' => 'blue' } }
+
+    it 'removes key if old experiment is found' do
+      @subject.cleanup_old_versions!(experiment)
+      expect(@subject.keys).to be_empty
+    end
+  end 
 
+  context '#cleanup_old_experiments!' do
     it 'removes key if experiment is not found' do
-      @subject.cleanup_old_experiments
+      @subject.cleanup_old_experiments!
       expect(@subject.keys).to be_empty
     end
 
     it 'removes key if experiment has a winner' do
       allow(Split::ExperimentCatalog).to receive(:find).with('link_color').and_return(experiment)
       allow(experiment).to receive(:start_time).and_return(Date.today)
       allow(experiment).to receive(:has_winner?).and_return(true)
-      @subject.cleanup_old_experiments
+      @subject.cleanup_old_experiments!
       expect(@subject.keys).to be_empty
     end
 
     it 'removes key if experiment has not started yet' do
       allow(Split::ExperimentCatalog).to receive(:find).with('link_color').and_return(experiment)
       allow(experiment).to receive(:has_winner?).and_return(false)
-      @subject.cleanup_old_experiments
+      @subject.cleanup_old_experiments!
       expect(@subject.keys).to be_empty
     end    
   end