def sort
if params[:fact_id]
@fact = Fact.find_by_id(params[:fact_id]) || Fact.new
@tags = current_user.facts.tag_counts
@collection = "facts"
@window_id = "new_fact_tags_window"
elsif params[:image_id]
@image = Image.find_by_id(params[:image_id]) || Fact.new
@tags = current_user.images.tag_counts
@collection = "images"
@window_id = "new_image_tags_window"
elsif params[:video_id]
@video = Video.find_by_id(params[:video_id]) || Video.new
@tags = current_user.videos.tag_counts
@collection = "videos"
@window_id = "new_fact_tags_window"
elsif params[:community_idea_id]
@community_idea = CommunityIdea.find_by_id(params[:community_idea_id]) || CommunityIdea.new
@tags = CommunityIdea.tag_counts
@collection = "community_ideas"
@window_id = "new_idea_tags_window"
elsif params[:stock_ad_id]
@tags = current_user.stock_ads.tag_counts
@collection = "stock_ads"
@stock_ad = StockAd.find_by_id(params[:stock_ad_id]) || StockAd.new
@window_id = "new_idea_tags_window"
elsif params[:image_id]
@tags = current_user.images.tag_counts
@collection = "images"
@image = Image.find_by_id(params[:image_id]) || Image.new
@window_id = "new_image_tags_window"
elsif params[:context] && (params[:context] == "community_ideas")
@tags = CommunityIdea.tag_counts
#@checked_tags = params[:tag_id] ? params[:tag_id].split(',') : []
@collection = "filters"
@window_id = "community_ideas_tags_window"
elsif params[:context] && (params[:context] == "facts")
@tags = Fact.tag_counts
#@checked_tags = params[:tag_id] ? params[:tag_id].split(',') : []
@collection = "filters"
@window_id = "modal_tags_window"
else
@tags = Tag.counts
@collection = ""
@window_id = ""
end
if params[:sort_by] == 'name'
@sorted_tags = @tags.sort{|t1,t2| t1.name <=> t2.name }
else
@sorted_tags = @tags.sort{|t1,t2| t2.count <=> t1.count }
end
respond_to do |format|
format.js
end
end
Refactorings
No refactoring yet !
danielharan
December 18, 2009, December 18, 2009 17:13, permalink
No rating.
Login to rate!
This smells of too many responsibilities being put in the same method. If you have VideosController, a tags method would be trivial, and you can let the view sort.
Line 8 instantiates a Fact by default. Is that right?
Anyhow, here are a couple elements that would help.
key = [:fact_id, :image_id, :video_id, :community_idea_id, :stock_ad_id].detect {|k| params.has_key?(k)}
if key
klass = key.classify.constantize
instance_variable_set key, (klass.find_by_id(params[key]) || klass.new)
@collection = key.to_s.sub(/_id$/, 's')
...
@sorted_tags = tags.sort_by(params[:sort_by] || 'count')
xybre
December 22, 2009, December 22, 2009 21:53, permalink
No rating.
Login to rate!
I saw this on your RSS feed and came in here to say pretty much what daniel already has, it really is the way to go. Make sure your conventions are consistent and it will make things so much easier when you're writing code like this.
mdarby.myopenid.com
January 7, 2010, January 07, 2010 02:52, permalink
No rating.
Login to rate!
I've very glad the refactoring I posted (and spent some time on) the day that this was posted was not posted. WTF?
I've very glad the refactoring I posted (and spent some time on) the day that this was posted was not posted. WTF?
Trying to make this a lot cleaner and leaner.