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.
['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>"
    else
      r[:label] = q.content
    end

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

    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 />')
    end

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

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

share|improve this question
    
is there a results = [] before the loop? are you missing an end at the end? is this a helper? –  tokland Dec 13 '12 at 18:34
    
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 –  regedarek Dec 13 '12 at 23:56

1 Answer 1

up vote 2 down vote accepted

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] }
end.uniq_by(&:first).take(MAX_RESULTS)

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))
  end
  {:title => q.content, :value => app_question_url(q.app, q.id), :label => label}
end

Notes:

  • 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.
share|improve this answer
    
Thanks a lot for your hints. –  regedarek Dec 13 '12 at 23:58
    
And since this is no helper I can use content_tag –  regedarek Dec 14 '12 at 10:11
1  
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. –  tokland Dec 14 '12 at 10:22
    
Hmm, Of course I can`t :), thx –  regedarek Dec 14 '12 at 10:31
    
"#{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 –  regedarek Dec 14 '12 at 12:23

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.