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 am using ElasticSearch and Tire for search in my app across all models.

  1. I would like to refactor autocomplete method because it looks to complex for me.
  2. Am I using a good pattern for searching across model?

Model

class Link < ActiveRecord::Base
  include Tire::Model::Search
  include Tire::Model::Callbacks

  tire.mapping do
    indexes :id, :type => 'string', :index => :not_analyzed
    indexes :title, analyzer: 'snowball', boost: 100
  end

  def to_indexed_json
    to_json(
      only: [:id, :title]
    )
  end
  ...
end

Controller

class SearchController < ApplicationController
  def new
    search = SearchesCatalog.new(params[:term])

    render json: search.autocomplete.to_json
  end
end

Lib

class SearchesCatalog
  attr_reader :options

  def initialize(options = {})
    @options = HashWithIndifferentAccess.new(options)
    @resources = [ :questions, :answers, :links, :events, past_events, :reviews ]
  end

  def results
    term = options[:term]

    Tire.search @resources do
      query { string "#{term}" }
    end.results
  end

  def autocomplete
    array = []
    results.each do |result|
      case result.type
    when "past_event"
      array << { title: result.content, label: "#{result.content} <span class='search-type'>PastEvent</span>", value: "/past_events/#{result.id}" }
    when "event"
      array << { title: result.title, label: "#{result.title} <span class='search-type'>Event</span>", value: "/events/#{result.id}" }
    when "topic"
      array << { title: result.name, label: "#{result.name} <span class='search-type'>Topic</span>", value: "/#{result.app_id}/t/#{result.id}" }
    when "link"
      array << { title: result.title, label: "#{result.title} <span class='search-type'>Link</span>", value: "/links/#{result.id}" }
    when "question"
      array << { title: result.content, label: "#{result.content[0..100]} <span class='search-type'>Question</span>" }
    when "answer"
    array << { title: result.content, label: "#{result.content[0..100]} <span class='search-type'>Answer</span>" }
    end
    array
  end
end

Javascript

// set up the search bar
$("#question_search").autocomplete({
  source:$('#question_search').data('source'),
  html: true,
  minlength: 1,
  appendto: "#search_results",
  select: function( event, ui ) {
    window.location=ui.item.value;
    return false;
  },
  focus: function( event, ui ) {
    $("#question_search").val(ui.item.title);
    return false;
  },
  open: function( event, ui ) {
    $('#search_results')
    .find('a:first').addclass('first').end()
    .find('a:last').addclass('last');
  }
});
share|improve this question

2 Answers 2

up vote 2 down vote accepted

I'll take autocomplete and leave the rest for others. Some notes:

  • Never write the pattern "empty array" + each + push + return array. That's a map (more on functional programming here)
  • Is this "lib" thing really in lib/? it should go to app/. In lib you only have generic code that may be reused across applications.
  • Don't write routes by hand, ever, that's terrible practice, use the helper router methods provided by Rails.
  • Don't write tags by hand, use content_tag.
  • Abstract and simplify by identifying repeated patterns in your code.

I'd write:

def autocomplete
  results.map do |result|
    title, caption, route = case result.type.to_sym
    when :past_event
      [result.content, "PastEvent", past_event_path(result)]
    ...
    end

    label = "%s %s" % [title, content_tag(:span, caption, class: 'search-type')]
    {title: title, label: label, value: route}
  end
end
share|improve this answer
    
Using routes looks great but I have problem when item not exist, for example: [result.content, "Review", service_provider_path(result.service_provider)] I would like to skip adding that item to hash if result.service_provider.nil? Is possible to not adding it to hash {title: title, label: label, value: route}? –  regedarek Dec 18 '12 at 14:47
    
write (service_provider_path(result.service_provider) if result.service_proder) –  tokland Dec 18 '12 at 14:51

One suggestion is to create three virtual attributes (title, label, link) for each and every model and add the custom implementations. Add these attributes to the index mapping.

Then you have a unique interface to all models. That way you can get rid of the case statement completely.

results.map do |result|
  {title: result.title, label: result.label, value: url}
end

It will make your index bigger but saves computation time on the autocomplete action.

share|improve this answer
1  
+1 That's the orthodox approach for OOP and introduces an interesting matter, the so-colled "Expression problem": c2.com/cgi/wiki?ExpressionProblem. Personally I've found that, when writing helpers, it's more maintainable to use cases that to scatter code throughout the models, though. In this case it should be more something like presenters, models are not really the place for "label" or "url" methods. –  tokland Dec 19 '12 at 7:41

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.