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 have following HTML class collector. Is there a better way than []tap for this?

def tr_classes(user)
  classes = [].tap do |c|
    c << "error" if user.company.nil?
    c << "disabled" if user.disabled?
  end
  if classes.any?
    " class=\"#{classes.join(" ")}\""
  end
end

<tr<%= tr_classes(user) %>>
  <td><%= user.name %></td>
</tr>
share|improve this question
add comment

2 Answers

up vote 3 down vote accepted

Notes:

  • This tap usage is not uncommon to see but IMHO it's very, very dubious. You can use a functional approach (see code below).
  • I wouldn't return the string along with the attribute, just the array of classes (which Rails3 helpers understand).

I'd write:

def tr_classes(user)
  [
    ("error" if !user.company),
    ("disabled" if user.disabled?),
  ].compact.presence
end

Tables are usually easier to build from helpers. You now would use the function that way:

content_tag(:tr, :class => tr_classes(user)) do
  content_tag(:td, user.name)
end
share|improve this answer
    
Glad somebody suggested content_tag; I find opening a <%= %> to mess with the individual attributes of an opening HTML tag to be particularly awful. –  meagar Dec 29 '12 at 12:37
add comment

What do you mean a "better way" than tap? Why are you using tap at all? Initialize an array. Conditionally append items to it. This pattern is as old as time, and there is no reason to shoe-horn in a Ruby-esque solution.

classes = [].tap do |c|
  c << "error" if user.company.nil?
  c << "disabled" if user.disabled?
end

vs

classes = []
classes << "error" if user.company.nil?
classes << "disabled" if user.disabled?

You've added so much complexity, both functionally and visually, for literally no gain. You've produced more and uglier code.

share|improve this answer
    
I also dislike this use of tap, but I guess that the OP is using it because it's not uncommon to see. The only good thing about this pattern is that side-effects are confined into a block, that maybe useful with other objects, with an array it's completely overkill. –  tokland Jan 2 '13 at 12:59
add comment

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.