3
\$\begingroup\$

I've got a simple sinatra webapp that pulls data from Pinboard's RSS api, parses the info, and re-displays it. There are 4 tasks I need to perform with the data:

  1. I need to remove all instances of a specific tag from all items (in this case, want)
  2. I need to create a list of all unique tags across all the items returned
  3. I need to dynamically remove certain tags from the list if they are already being used to filter the list
  4. I need to add an attribute to each item that is the host of the url associated with that item (i.e. amazon.com for whatever long amazon url may have been added)
  5. I need to return a list of unique locations across all the items returned

Here is the way I am currently handling this:

items = []
tags = []
locations = []
begin
  JSON.parse(data).each do |item|
    # Remove the tag want from the list of tags,
    # strip any extra whitespace, and add a location attribute
    item['t'].each { |t| t.strip! }
    item['t'].delete_if { |tag| tag == 'want' }

    # Try like hell to parse the url. Assign an error string as a last resort
    begin
      item['l'] = /https?:\/\/(?:[-\w\d]*\.)?([-\w\d]*\.[a-zA-Z]{2,3}(?:\.[a-zA-Z]{2})?)/i.match(item['u'])[1]
    rescue
      item['l'] = "URL Parse error"
    end

    # Add the item's tags
    item['t'].each do |tag|
      tags << tag unless tags.include? tag or filter_tags.include? tag
    end

    locations << item['l'] unless locations.include? item['l']

    items << item
  end
rescue
end
return items, tags.sort, locations.sort

I really don't like this. It just doesn't feel clean to me, what with the creation of the empty arrays, and adding stuff in, etc. But my thought was that it's better to do a single iteration over the data returned than to do multiple loops. But now I am considering this instead:

items = JSON.parse(data)
# Remove the want tag from all items
items.each { |i| i['t'].delete_if { |t| t == 'want' }}

#generate a list of tags
tags = items.map { |i| i['t']}.flatten.uniq.sort
tags.delete_if { |t| filter_tags.include? t }

# Parse the url
re = /https?:\/\/(?:[-\w\d]*\.)?([-\w\d]*\.[a-zA-Z]{2,3}(?:\.[a-zA-Z]{2})?)/i
items.each { |i| i['l'] = re.match(i['u'])[1] }

# Generate a list of locations
locations = items.map { |i| i['l']}.flatten.uniq.sort

return items, tags, locations

It feels cleaner, and seems like it's easier to read, but I'm not sure if the nested delete_if in the first bit is going too far. I'm also not sure how I feel about iterating over the items multiple times, or chaining flatten.uniq.sort for both tags and locations

\$\endgroup\$
4
\$\begingroup\$

The second version is undoubtedly better, more readable, even if you iterate multiple times over the data.

The problem is: both versions, specially the first, suffer from a common problem: imperative style ("do this, do that"). Are you familiar with the difference between imperative (focus on statements and state change) and functional programming (focus on expressions and immutable data)? it's easy, just try to code without in-place updates (no each, delete_if, bang! methods, ...) and you'll see that you'll end up writing less code, easier to test, to debug and to understand. You'll pay a bit of memory/speed performance, but it's usually worth it.

Now, using a functional approach and taking advantage of existing libraries (uri), that's what I'd write (some details may not be right, but you get the idea):

require 'uri'

def parse(data, filter_tags)
  items, nested_tags, nested_locations = JSON.parse(data).map do |item|
    location = URI.parse(item["l"]).host rescue "URL Parse error"
    tags = item["t"].map(&:strip) - filter_tags
    [item, tags, location]
  end.transpose

  [items, nested_tags.flatten(1).uniq.sort, nested_locations.flatten(1).uniq.sort]
end

More on functional programming with Ruby: http://code.google.com/p/tokland/wiki/RubyFunctionalProgramming

\$\endgroup\$
4
  • \$\begingroup\$ Awesome. Thanks for the tips. I'll definitely be reading through your linked doc. The only thing this isn't doing is removing the tag from the item's tags. Is there a better way to do that, other than item['t'].delete_if { |t| t == 'want' }? It seems like that's the one thing I have to edit in place. \$\endgroup\$ – Gordon Fontenot Aug 3 '12 at 16:05
  • \$\begingroup\$ @Gordon. No need to use delete_if, there is reject. Note though that you can create a new variable holding filter_tags + ["want"], and now use it instead of filter_tags. \$\endgroup\$ – tokland Aug 3 '12 at 16:08
  • \$\begingroup\$ Not trying to start a discussion in the comments, but if I don't edit item['t'] in place, then when I iterate over the items in the view, I'd have to pass another array down the line, or add another attribute to the item. So rather than doing that, wouldn't delete_if or reject be simpler? sorry if there's something I'm missing here. \$\endgroup\$ – Gordon Fontenot Aug 3 '12 at 16:22
  • \$\begingroup\$ @GordonFontenot: ok, now I understand what you mean. The solution is not hard: make all the processing you need at this point; instead of returning item values unchanged, create new ones that hold the values you expect in the view. That way you don't need to update anything. \$\endgroup\$ – tokland Aug 3 '12 at 16:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.