# Refactoring a HTML class collector?

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>

-

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

-
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

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.

-
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