2
\$\begingroup\$

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

    ## Download the files
    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|
      file_data = File.read(File.join(self.xml_dir, 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.

\$\endgroup\$
2
\$\begingroup\$
  • 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/'
    def download_files(uri: DEFAULT_URI, ...)
    
  • 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.download_files(url: ..., limit: 1)
    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.

\$\endgroup\$

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.