def user_photo(user, values = {})
values[:style] ||= ''
values[:class] ||= ''
values[:onmouseover] ||= "show_reputation_flyover(#{values[:reputation_flyover_id]})" if App.reputations? && values[:reputation_flyover_id]
values[:onmouseout] ||= "cancel_reputation_flyover(#{values[:reputation_flyover_id]})" if App.reputations? && values[:reputation_flyover_id]
return image_tag("#{user.screen_name}.gif", :alt => user.screen_name) if user && user.proxy_user?
return image_tag("reviewer_null.gif", :alt => user.screen_name) if user.blank? || user.account_status.to_s == "suspended" || user.account_status.to_s == "deleted"
photo = user.primary_photo
if photo
options = { :class => "user_thumb#{" " + values[:class]}",
:title => "#{h(user.screen_name)}",
:style => "background: url('#{photo.get_cropped_url}') no-repeat",
:rel => "nofollow" }
code = link_to '', profile_url(:screen_name => user.screen_name, :only_path => values[:only_path]), options
code << render_to_string(:partial => "reputable/reputation_flyover", :object => user, :locals => { :reputation_flyover_id => values[:reputation_flyover_id] }) if values[:reputation_flyover_id]
else
code = link_to(image_tag("reviewer_null.gif", :alt => user.screen_name),
profile_url(:screen_name => user.screen_name), :rel => "noindex,nofollow", :onmouseover => values[:onmouseover], :onmouseout => values[:onmouseout])
code << render_to_string(:partial => "reputable/reputation_flyover", :object => user, :locals => { :reputation_flyover_id => values[:reputation_flyover_id] }) if values[:reputation_flyover_id]
end
code
end
Refactorings
No refactoring yet !
Doug Johnston
October 4, 2010, October 04, 2010 17:13, permalink
No rating.
Login to rate!
You could start by moving some things into the model (example below). That will clean up some of your conditionals. But, in a larger sense, you should think about working towards unobtrusive JS and RESTful controller actions to handle this. I used to use complex helpers for this sort of thing, but eventually discovered that the combination of partials, RESTful controllers, and moving logic to my models was a pretty powerful approach and made for a cleaner, more manageable code base.
# Move this stuff into your model user.account_status.to_s == "suspended" # Becomes... user.suspended? # In user.rb def suspended? account_status.to_s == "suspended" end
Bit of a Ruby noob. Could use some help refactoring this little bit.