# Web scraper extracts zip files and publishes them to redis

I wrote this program for a job interview a while back. I would like general feedback on style, readability, maintainability, or any obvious flaws:

 #!/usr/bin/ruby
require 'httparty'
require 'nokogiri'
require 'fileutils'
require 'zip'
require 'redis'

Zip.on_exists_proc = true ## When extracting zip files, old files will be overwritten.

class Scraper
attr_accessor :uri
attr_accessor :xml_dir
attr_accessor :zip_dir
attr_accessor :redis_key

def initialize(uri: "http://feed.omgili.com/5Rh5AMTrc4Pv/mainstream/posts/", zip_dir: "tmp", xml_dir: "xml", redis_key: "NEWS_XML", limit: nil)
## Set some vars
@uri = uri
@zip_dir = zip_dir
@xml_dir = xml_dir
@redis_key = redis_key
@limit = limit

## Get the filenames
begin
page_obj = Nokogiri::HTML(HTTParty.get(uri))
rescue HTTParty::Error
puts "Error connecting to #{uri}. It may be that your connection is down, or that the address is bad."
puts "Exiting..."
exit
rescue StandardError
puts "Error connecting to #{uri}. It may be that your connection is down, or that the address is bad."
puts "Exiting..."
exit
end

filenames = page_obj.css('td a').map {|node| node['href'] }.select {|attr| attr.include? ".zip"} ## Exclude anything that doesn't end with '.zip'
filenames = filenames[0..limit-1] if limit.is_a?(Integer) && limit-1 >= 0
zip_uris = filenames.map {|filename| uri + filename}
filenames_to_uris = [filenames, zip_uris].transpose.to_h

Dir.mkdir(zip_dir) if !Dir.exist? zip_dir
filenames_to_uris.each do |name, uri|
zip_data = HTTParty.get(uri).body
File.write("#{zip_dir}/#{name}", zip_data)
end
end

def extract_zip()
Dir.mkdir(self.xml_dir) if !Dir.exists? self.xml_dir
Dir.foreach(self.zip_dir).drop(2).each do |filename| ## .drop(2) because the first two elements are . and ..
file_path = File.join(self.zip_dir, filename)
Zip::File.open(file_path) do |zipped_files|
zipped_files.each do |file|
file.extract(File.join(self.xml_dir, file.name))
end
end
end
end

def push_xml_to_redis()
redis = Redis.new()
file_list = Dir.foreach("#{self.xml_dir}").drop(2) ## .drop(2) because the first two elements are . and ..
file_list.each do |filename|
redis.hset(self.redis_key, filename, file_data)
end
end
end


Where an example of usage would be:

s = Scraper.new(limit: 1)
s.extract_zip()
s.push_xml_to_redis()


The full project can be found on github.

• It's not generally recommended to do a lot of work in the initializer, I would move the code to download the files into a separate method.

• Normal Ruby style is usually to not use parentheses when no parameters are passed to methods, i.e. s.extract_zip not s.extract_zip().

• It's usually frowned on to do type comparisons in Ruby like this:

filenames = filenames[0..limit-1] if limit.is_a?(Integer) && limit-1 >= 0


so either assume that the user passes a valid value or nil (this is what I would do):

filenames = filenames[0..limit-1] if limit && limit > 0


or just do a straight conversion:

filenames = filenames[0..limit.to_i-1] if limit.to_i >= 0

• Use constants for magic values,

DEFAULT_URL = 'uri: "http://feed.omgili.com/5Rh5AMTrc4Pv/mainstream/posts/'

• Use single quotes for strings that don't sue replacements or escaped characters (\n)

• I would also not require parameters on the initializer that are just stored for use in one method (unless you expect to be calling it multiple times) so my usage would more likely be:

s = Scraper.new
s.extract_zip(zip_dir: ....)
s.push_xml_to_redis(xml_dir: ....)

• However you might want to balance this with not expecting the user of your class to not have to call so many methods, or know so much, to call your class. As such I might design this for usage like:

s = Scraper.new
s.uri = ...
s.xml_dir = ...
s.scrape!

• If I was an interviewer, I would even be more impressed if you separated out your logic into a class for scraping and one for writing (which could easily be replaced with a different backing store, say s3 or file-system based)

class RedisStore
def initialize(redis_key)
...
end
def store!(filename, data)
@redis.hset(self.redis_key, filename, data)
end
end
class Scraper
...
def push_xml_to_store(store)
...
file_list.each do |filename|
store.store!(filename, file_data)
end
end
end

store = RedisStore.new
scraper.push_xml_to_store(store)


this would also make testing either.

• One last point. It would be good to do this without creating temporary files. This would simplify usage because there would be no need to worry about setting the correct zip_dir and xml_dir or cleaning up afterwards.