Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
['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?
    matched_qs <<

    r = {:title => q.content, :value => app_question_url(,}
    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,].compact.join('<br />')

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

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 – regedarek Dec 13 '12 at 23:56
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), 
  }) { |q| [q, type] }

results = 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(,, :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.
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
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/> – regedarek Dec 14 '12 at 12:23

Your Answer


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.