# Filtering Pinboard API results by tags

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

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

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

• 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. – Gordon Fontenot Aug 3 '12 at 16:05
• @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. – tokland Aug 3 '12 at 16:08
• 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. – Gordon Fontenot Aug 3 '12 at 16:22
• @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. – tokland Aug 3 '12 at 16:25