Refactoring a HTML class collector?

Question

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>

Answer 1

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

• user229044: Glad somebody suggested content_tag; I find opening a <%= %> to mess with the individual attributes of an opening HTML tag to be particularly awful.

Answer 2

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.

• tokland: 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.