74198b06bc9e68739debfe6c23e070d3

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?

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 !

F1e3ab214a976a39cfd713bc93deb10f

Tj Holowaychuk

March 30, 2009, March 30, 2009 23:40, permalink

No rating. Login to rate!

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">]
74198b06bc9e68739debfe6c23e070d3

k776

March 30, 2009, March 30, 2009 23:46, permalink

No rating. Login to rate!

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
74198b06bc9e68739debfe6c23e070d3

k776

March 30, 2009, March 30, 2009 23:47, permalink

No rating. Login to rate!

(credits to k1mmeh at http://snippets.dzone.com/posts/show/6608 for the one line optimization)

F1e3ab214a976a39cfd713bc93deb10f

Tj Holowaychuk

March 31, 2009, March 31, 2009 00:12, permalink

No rating. Login to rate!

still pretty messy IMO

74198b06bc9e68739debfe6c23e070d3

k776

March 31, 2009, March 31, 2009 06:41, permalink

No rating. Login to rate!

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
F1e3ab214a976a39cfd713bc93deb10f

Tj Holowaychuk

March 31, 2009, March 31, 2009 15:15, permalink

No rating. Login to rate!

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

A8d3f35baafdaea851914b17dae9e1fc

Adam

April 1, 2009, April 01, 2009 05:07, permalink

No rating. Login to rate!
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
F1e3ab214a976a39cfd713bc93deb10f

Tj Holowaychuk

April 1, 2009, April 01, 2009 15:19, permalink

No rating. Login to rate!

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

A8d3f35baafdaea851914b17dae9e1fc

Adam

April 1, 2009, April 01, 2009 15:39, permalink

No rating. Login to rate!

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
F1e3ab214a976a39cfd713bc93deb10f

Tj Holowaychuk

April 1, 2009, April 01, 2009 16:23, permalink

No rating. Login to rate!

Haha :P sorry just seems really needless, thats the reason for the attr_* methods

A8d3f35baafdaea851914b17dae9e1fc

Adam

April 1, 2009, April 01, 2009 16:31, permalink

No rating. Login to rate!

I like to make extensive use of Struct on refactormycode because it hides boring implementation details, letting the reader focus on the actual refactoring.

F1e3ab214a976a39cfd713bc93deb10f

Tj Holowaychuk

April 1, 2009, April 01, 2009 17:47, permalink

No rating. Login to rate!

True good point, ive just seen it lots in production code as well with huge arrays of symbols looking all nasty

Your refactoring





Format Copy from initial code

or Cancel