How to refactor this complicated html generator in Ruby?

Question

['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]

• tokland: is there a results = [] before the loop? are you missing an end at the end? is this a helper?
• tomekfranek: 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

Answer 1

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.

• tomekfranek: And since this is no helper I can use content_tag
• tokland: 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.
• tomekfranek: Hmm, Of course I can`t :), thx
• tomekfranek: "#{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