1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33
class Item def raw_tag_list; "one, two, three, four, five"; end def public_tags; [<#Tag name=two>, <#Tag name=five>, <#Tag name=one>, <#Tag name=four>, <#Tag name=three>]; end def tags return Array.new if raw_tag_list.nil? # Get the raw tag list, split, squish (removed whitespace), and add each to raw_tag_array # Make sure we skip if the array already has that tag name (remove any duplicates that occur) raw_tag_array = Array.new raw_tag_list.split(',').each do |raw_tag| next if raw_tag_array.include?(raw_tag.squish) raw_tag_array << raw_tag.squish end tags = Array.new # grab all the tag objects tags_out_of_order = public_tags if tags_out_of_order.size > 0 # resort them to match raw_tag_list order raw_tag_array.each do |tag_name| tag = tags_out_of_order.select { |tag| tag.name == tag_name } tags << tag end # at this point, we have an array, with arrays of object [[tag], [tag], [tag]] # use compact to remove any nil objects, and flatten to convert it to [tag, tag, tag] tags = tags.compact.flatten end tags end end
Refactorings
No refactoring yet !
Tj Holowaychuk
March 30, 2009, March 30, 2009 23:40, permalink
I dont know exactly what your trying to accomplish, but its wrong :p
1 2 3 4 5 6 7 8 9 10 11 12 13
class Tag def initialize name @name = name end def self.parse string string.split(',').map { |tag| Tag.new(tag.strip) } end end params = { :tags => 'foo,bar, some more bar, whoop' } p Tag.parse(params[:tags]) # => [#<Tag:0x27a88 @name="foo">, #<Tag:0x27a60 @name="bar">, #<Tag:0x27a38 @name="some more bar">, #<Tag:0x27a10 @name="whoop">]
k776
March 30, 2009, March 30, 2009 23:46, permalink
Thanks for trying. I ended up going with the following or now. Note the sort replacing 5 lines. The idea is to take an unordered sql query result from Rails, and order it according to a comma separated string.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
class Item def raw_tag_list; "one, two, three, four, five"; end def public_tags; [<#Tag name=two>, <#Tag name=five>, <#Tag name=one>, <#Tag name=four>, <#Tag name=three>]; end def tags return Array.new if raw_tag_list.nil? # Get the raw tag list, split, squish (removed whitespace), and add each to raw_tag_array # Make sure we skip if the array already has that tag name (remove any duplicates that occur) raw_tag_array = Array.new raw_tag_list.split(',').each do |raw_tag| next if raw_tag_array.include?(raw_tag.squish) raw_tag_array << raw_tag.squish end tags = Array.new # grab all the tag objects tags_out_of_order = public_tags if tags_out_of_order.size > 0 # resort them to match raw_tag_list order tags = tags_out_of_order.sort { |a, b| raw_tag_array.index(a.name).to_i <=> raw_tag_array.index(b.name).to_i } ### <----- end tags end end
k776
March 30, 2009, March 30, 2009 23:47, permalink
(credits to k1mmeh at http://snippets.dzone.com/posts/show/6608 for the one line optimization)
k776
March 31, 2009, March 31, 2009 06:41, permalink
Got it down to this. Can you do better?
1 2 3 4 5 6 7 8 9 10 11 12
class Item def raw_tag_list; "one, two, three, four, five"; end def public_tags; [<#Tag name=two>, <#Tag name=five>, <#Tag name=one>, <#Tag name=four>, <#Tag name=three>]; end def tags return Array.new if raw_tag_list.blank? raw_tag_array = raw_tag_list.squish.split(/\s?,\s?/).uniq tags = public_tags tags.sort! { |a, b| raw_tag_array.index(a.name).to_i <=> raw_tag_array.index(b.name).to_i } if tags.size > 0 tags end end
Tj Holowaychuk
March 31, 2009, March 31, 2009 15:15, permalink
Just seems like a really hackish thing to do even having hardcoded orders like that, and sorting like that. Maybe consider using the Comparable mixin or abstract the sorting to a different method. Squishing it all into #tags will just slow it down and prevent you from using that functionality later
Adam
April 1, 2009, April 01, 2009 05:07, permalink
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25
require 'Linguistics' class Tag < Struct.new(:name) NUMBERS = (0..100).inject(Hash.new(1 / 0.0)) do |hash,number| hash.merge(Linguistics::EN.numwords(number) => number) end def to_i NUMBERS[name] end def <=>(tag) to_i <=> tag.to_i end end class Item def public_tags [ Tag.new('eight'), Tag.new('five'), Tag.new('ten'), Tag.new('foo'), Tag.new('one') ] end def tags public_tags.sort end end
Tj Holowaychuk
April 1, 2009, April 01, 2009 15:19, permalink
Not a huge fan of people using Struct all the time, adding another class to the chain JUST to add one attribute is pretty bad practice IMO, but at least you were able to figure out what he wanted lol
Adam
April 1, 2009, April 01, 2009 15:39, permalink
Refactored to eliminate the inheritance because Tj doesn't like it extra classes. ;)
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
require 'Linguistics' Tag = Struct.new(:name).class_eval do NUMBERS = (0..100).inject(Hash.new(1 / 0.0)) do |hash,number| hash.merge(Linguistics::EN.numwords(number) => number) end def to_i NUMBERS[name] end def <=>(tag) to_i <=> tag.to_i end self end class Item def public_tags [ Tag.new('eight'), Tag.new('five'), Tag.new('ten'), Tag.new('foo'), Tag.new('one') ] end def tags public_tags.sort end end
Tj Holowaychuk
April 1, 2009, April 01, 2009 16:23, permalink
Haha :P sorry just seems really needless, thats the reason for the attr_* methods
Adam
April 1, 2009, April 01, 2009 16:31, permalink
I like to make extensive use of Struct on refactormycode because it hides boring implementation details, letting the reader focus on the actual refactoring.
Tj Holowaychuk
April 1, 2009, April 01, 2009 17:47, permalink
True good point, ive just seen it lots in production code as well with huge arrays of symbols looking all nasty
Is there a faster way to do this? I need to sort an array of tags according to the order of tags in a string. At the moment, I make the array then select as go through. But this is slow. Is there a fancy ruby alternative?