Refactor recommendations (#463)

* Refactor recommendations

* Hound happiness 🐶

* Use `find`

* Remove unreachable code

* Add a newline

* Load comments through association

* Add spaces

* Make recommendation lookup instance methods

* Woof! 🐶

* Move things around

app/controllers/phenotypes_controller.rb

@@ -83,81 +83,32 @@ class PhenotypesController < ApplicationController
   end
 
   def show
-    @phenotype = Phenotype.find(params[:id]) || not_found
-    @comments = PhenotypeComment
-      .where(phenotype_id: params[:id])
-      .order('created_at ASC')
+    @phenotype = Phenotype.find(params[:id])
+    @comments = @phenotype
+                .phenotype_comments
+                .order('created_at ASC')
     @phenotype_comment = PhenotypeComment.new
     @user_phenotype = UserPhenotype.new
-
-    recommender = PhenotypeRecommender.new
-    similar_ids = recommender.for(params[:id])
-    # For some reason, Recommendify sometimes returns items of class Integer,
-    # sometimes of class Recommendify.
-    if similar_ids[0].is_a?(Recommendify::Neighbor)
-      similar_ids = similar_ids.map(&:item_id)
-    end
-    @similar_phenotypes = Phenotype
-                          .where('id in (?)', similar_ids)
-                          .limit(6)
+    @similar_phenotypes =
+      PhenotypeRecommender.new.recommendations_for(@phenotype.id, 6)
   end
 
   def recommend_phenotype
-    # init the recommendation-engines
-    @phenotype_recommender = PhenotypeRecommender.new
-    @variation_recommender = VariationRecommender.new
+    @phenotype = Phenotype.find(params[:id])
 
     # get up to three similar phenotypes regardless of variation
-
-    @similar_ids = @phenotype_recommender.for(params[:id])
-    @similar_phenotypes = []
-    @item_counter = 0
-
-    @similar_ids.each do |s|
-      if @item_counter < 3
-        @phenotype = Phenotype.find(s.item_id)
-        if current_user.phenotypes.include?(@phenotype) == false
-          @similar_phenotypes << @phenotype
-          @item_counter += 1
-        end
-      else
-        break
-      end
-    end
+    @similar_phenotypes =
+      PhenotypeRecommender.new.recommendations_for(@phenotype.id, 3)
 
     # get up to three similar combinations of phenotype and variation
-    @user_phenotype = UserPhenotype.find_by_phenotype_id_and_user_id(params[:id],current_user.id)
-    if @user_phenotype != nil
-      @users_variation = @user_phenotype.variation
-      @variation_recommend_request = params[:id] + '=>' + @users_variation
-    else
-      @variation_recommend_request = ''
-    end
+    @user_phenotype = @phenotype
+                      .user_phenotypes
+                      .find_by(user_id: current_user.id)
+    @similar_variations =
+      VariationRecommender.new.recommendations_for(@user_phenotype, 3)
 
-    @similar_combinations = @phenotype_recommender.for(@variation_recommend_request)
-    @similar_variations = []
-    @combination_counter = 0
-
-    @similar_combinations.each do |s|
-      if @combination_counter < 3
-        @phenotype = Phenotype.find_by(id: s.item_id.split('=>')[0])
-        if current_user.phenotypes.include?(@phenotype) == false
-          @similar_variations << s
-          @combination_counter += 1
-        end
-      else
-        break
-      end
-    end
-
-    @phenotype = Phenotype.find_by(id: params[:id])
-
-    if @similar_phenotypes == [] and @similar_variations == []
+    if @similar_phenotypes.none? && @similar_variations.none?
       redirect_to action: 'index'
-    else
-      respond_to do |format|
-        format.html
-      end
     end
   end
 

app/recommenders/phenotype_recommender.rb

@@ -17,4 +17,14 @@ class PhenotypeRecommender < Recommendify::Base
 
     process!
   end
+
+  def recommendations_for(id, count)
+    phenotype_ids = self.class
+                        .new
+                        .for(id)
+                        .take(count)
+                        .map(&:item_id)
+    return [] if phenotype_ids.empty?
+    Phenotype.find(phenotype_ids)
+  end
 end

app/recommenders/variation_recommender.rb

@@ -18,4 +18,40 @@ class VariationRecommender < Recommendify::Base
     process!
   end
 
+  def recommendations_for(user_phenotype, count)
+    neighbors = self.class
+                    .new
+                    .for("#{user_phenotype.phenotype_id}=>#{user_phenotype.variation}")
+                    .take(count)
+    phenotype_ids = neighbors.map(&method(:phenotype_id_from_neighbor))
+    phenotypes = Phenotype.find(phenotype_ids).index_by(&:id)
+
+    neighbors.map do |neighbor|
+      phenotype = phenotypes.fetch(phenotype_id_from_neighbor(neighbor))
+      Recommendation.new(neighbor, phenotype)
+    end
+  end
+
+  private
+
+  def phenotype_id_from_neighbor(neighbor)
+    neighbor.item_id.split('=>').first.to_i
+  end
+
+  class Recommendation
+    attr_reader :phenotype
+
+    def initialize(neighbor, phenotype)
+      @neighbor = neighbor
+      @phenotype = phenotype
+    end
+
+    def variation
+      neighbor.item_id.split('=>').last
+    end
+
+    private
+
+    attr_reader :neighbor
+  end
 end

app/views/phenotypes/recommend_phenotype.html.erb

@@ -1,30 +1,61 @@
-<%if @similar_variations != []%>
-<h2>Similar Variations</h2>
-You have just entered that <em><strong><%=@users_variation%></strong></em> is your variation for the phenotype <em><strong><%=@phenotype.characteristic%></strong></em>. Below you can find <%=@similar_variations.size%> <%if @similar_variations.size > 1%>phenotypes and the answers which are<%else%>phenotype and the answer which is<%end%> most-often entered by users who also gave <em><%=@users_variation%></em> as their variation for <em><%=@phenotype.characteristic%></em>. You haven't entered information about this phenotype yet. Do you have the same variation for the phenotype or something completely different? Let us know!<br/><br/>
-<div class="row">
-<%@similar_variations.each do |s|%>
-	<div class="span<%=(16/@similar_variations.size).to_i%> columns">
-		<div class="alert alert-block alert-success" data-alert="alert">
-			<h5><%=link_to Phenotype.find_by_id(s.item_id.split("=>")[0]).characteristic,Phenotype.find_by_id(s.item_id.split("=>")[0])%></h5>
-			Users with phenotypic variation similar to yours often gave <em><strong><%=s.item_id.split("=>")[1]%></strong></em> as variation for this phenotype. What about you?
-			</div>
-		</div>
-	<%end%>
-</div> 
-<%end%>
+<% if @similar_variations.any? %>
+  <h2>Similar Variations</h2>
+  You have just entered that
+  <em><strong><%= @user_phenotype.variation %></strong></em>
+  is your variation for the phenotype
+  <em><strong><%= @phenotype.characteristic %></strong></em>.
+  Below you can find
+  <%= @similar_variations.count %>
+  <% if @similar_variations.size > 1 %>
+    phenotypes and the answers which are
+  <% else %>
+    phenotype and the answer which is
+  <% end %>
+  most-often entered by users who also gave
+  <em><%= @user_phenotype.variation %></em> as their variation for
+  <em><%= @phenotype.characteristic %></em>. You haven't entered information
+  about this phenotype yet. Do you have the same variation for the phenotype or
+  something completely different? Let us know!<br/><br/>
+  <div class="row">
+  <% @similar_variations.each do |v| %>
+    <div class="span<%= 16 / @similar_variations.count %> columns">
+      <div class="alert alert-block alert-success" data-alert="alert">
+        <h5><%= link_to(v.phenotype.characteristic, v.phenotype) %></h5>
+        Users with phenotypic variation similar to yours often gave
+        <em><strong><%= v.variation %></strong></em> as variation for this
+        phenotype. What about you?
+        </div>
+      </div>
+    <% end %>
+  </div> 
+<% end %>
 
-<% if @similar_phenotypes != []%>
-<h2>Similar Phenotypes</h2>
-Below you can find <%=@similar_phenotypes.size%> <%if @similar_phenotypes.size > 1%>phenotypes<%else%>phenotype<%end%> which <%if @similar_phenotypes.size > 1%>are<%else%>is<%end%> often entered by users who provided us with any information about <em><strong><%=@phenotype.characteristic%></strong></em>. This list ignores the specific answers that have been given about phenotypes. Maybe you also want to enter information about this?<br/><br/>
-<div class="row">
-<%@similar_phenotypes.each do |s|%>
-	<div class="span<%=(16/@similar_phenotypes.size).to_i%> columns">
-		<div class="alert alert-block alert-success" data-alert="alert">
-		<h5><%=link_to s.characteristic,s%></h5>
-		Description: <em><%=simple_format(s.description)%></em>
-		</div>
-	</div>
-<%end%>
-</div>
-
-<%end%>
+<% if @similar_phenotypes.any? %>
+  <h2>Similar Phenotypes</h2>
+  Below you can find <%=@similar_phenotypes.size %>
+  <% if @similar_phenotypes.count > 1 %>
+    phenotypes
+  <% else %>
+    phenotype
+  <% end %>
+  which
+  <% if @similar_phenotypes.count > 1 %>
+    are
+  <% else %>
+    is
+  <% end %>
+  often entered by users who provided us with any information about
+  <em><strong><%= @phenotype.characteristic %></strong></em>. This list ignores
+  the specific answers that have been given about phenotypes. Maybe you also
+  want to enter information about this?<br/><br/>
+  <div class="row">
+  <% @similar_phenotypes.each do |p| %>
+    <div class="span<%= 16 / @similar_phenotypes.count %> columns">
+      <div class="alert alert-block alert-success" data-alert="alert">
+      <h5><%= link_to(p.characteristic, p) %></h5>
+      Description: <em><%= simple_format(p.description) %></em>
+      </div>
+    </div>
+  <% end %>
+  </div>
+<% end %>