['content', 'answers', 'comments'].each do |t|
  break if results.length >= MAX_RESULTS
  Question.search_tank('', :conditions => conditions.merge(t => "#{search_term}"), :page => 1, :per_page => 5).each do |q|
    next if matched_qs.include? q.id
    matched_qs << q.id

    r = {:title => q.content, :value => app_question_url(q.app, q.id)}
    if t == 'content'
      r[:label] = highlight_matched(search_regex, q.content)
      r[:label] << "<span class='search-type'>Question</span>"
      r[:label] = q.content

    if t == 'answers'
      r[:label] = "<span class='search-type snippet'>Answer</span>" + [r[:label], get_snippet(search_regex, q.answers)].compact.join('<br />')

    if t == 'comments'
      r[:label] = "<span class='search-type snippet'>Comment</span>" + [r[:type], r[:label], get_snippet(search_regex, q.answers.map(&:comments).flatten)].compact.join('<br />')

    results << r
    break if results.length >= MAX_RESULTS

I would like to refactor that part which generates r[:label]

  • \$\begingroup\$ is there a results = [] before the loop? are you missing an end at the end? is this a helper? \$\endgroup\$ – tokland Dec 13 '12 at 18:34
  • \$\begingroup\$ Yes there is results = [] before the loop and yes i miss end No this is no helper but search#new action which render json results.to_json gist.github.com/6738554e438c741fba45 \$\endgroup\$ – tomekfranek Dec 13 '12 at 23:56

This needs some tweaking, but I hope it helps you identify the main issues to address in your code (IMHO):

objects = [:content, :answers, :comments].flat_map do |type|
  questions = Question.search_tank('', {
    :page => 1, 
    :per_page => 5,
    :conditions => conditions.merge(type => search_term), 
  questions.map { |q| [q, type] }

results = objects.map do |q, type| 
  label = case type
  when :content
    highlight_matched(search_regex, q.content) + 
      content_tag(:span, "Question", :class => "search-type")
  when :answers
    content_tag(:span, "Answer", :class => 'search-type snippet'>) + 
      [q.content, get_snippet(search_regex, q.answers)].compact.join(tag(:br))
  when :comments
    comments = get_snippet(search_regex, q.answers.flat_map(&:comments))]    
    content_tag(:span, "Comment", :class => 'search-type snippet'>) +
      [t, q.content, comments].compact.join(tag(:br))
  {:title => q.content, :value => app_question_url(q.app, q.id), :label => label}


  • Avoid stateful variables whenever possible (that's it, don't update inplace), work with expressions.
  • Use Rails helpers (content_tag, tag, ...).
  • Use symbols for key-like values, not strings.
  • "#{something}" -> something.to_s
  • map + flatten = flat_map. Even though you should able to write q.answer_commments in Rails 3.1 will the correct model settings.
  • Use case, not a chain of if/elsif if the condition is on the same value.
  • \$\begingroup\$ And since this is no helper I can use content_tag \$\endgroup\$ – tomekfranek Dec 14 '12 at 10:11
  • 1
    \$\begingroup\$ can or can't? it does not matter if it's not a helper, you can always use helpers just with an include of the desired module. \$\endgroup\$ – tokland Dec 14 '12 at 10:22
  • \$\begingroup\$ Hmm, Of course I can`t :), thx \$\endgroup\$ – tomekfranek Dec 14 '12 at 10:31
  • \$\begingroup\$ "#{content_tag(:span, "Answer", :class: 'search-type snippet')}" + [q.content, get_snippet(search_regex, q.answers)].compact.join(tag(:br)) I dont know why I have to use #{} here to_s not working and i see html tags <br/> i.imgur.com/81mst.png \$\endgroup\$ – tomekfranek Dec 14 '12 at 12:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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