1
\$\begingroup\$

I wrote a little ruby script to download issues of PragPub magazine and send them to my Kindle via email. I would love to get some feedback on it.

One part, specifically, where I feel like there might be a better way is where it downloads the file and writes to disk. It used to do this:

open(filepath, 'wb') do |file|
  file << open(url).read
end

But this was causing a problem when the issue was not available yet. The http request would fail and there would be an empty file left on disk. So I changed it to this:

  begin
    content = open(url).read
  rescue OpenURI::HTTPError => e
    abort "Unable to download #{filename}: #{e.message}"
  end
  open(filepath, 'wb') do |file|
    file << content
  end

This works better, but feels a little clunky.

Also, is it common/acceptable in Ruby-land to just write a procedural style script like this, or should it be organized into class(es) and methods?

pragpub2kindle.rb

#!/usr/bin/env ruby

require 'open-uri'
require 'mail'
require 'optparse'

options = {}
optparse = OptionParser.new do |opts|
  opts.banner = "Usage: pragpub2kindle.rb [options] [yyyy-mm]"
  options[:mailto] = nil
  opts.on('-t', '--mailto EMAIL', 'Send attachment to email address') do |email|
    options[:mailto] = email
  end
  options[:mailfrom] = nil
  opts.on('-f', '--mailfrom EMAIL', 'Send attachment from email address') do |email|
    options[:mailfrom] = email
  end
  options[:dir] = nil
  opts.on('-d', '--dir DIR', 'Directory to save files') do |dir|
    options[:dir] = dir
  end
  opts.on('-h', '--help', 'Display this screen') do
    puts opts
    exit
  end
end

optparse.parse!

mandatory = [:mailto, :mailfrom]
missing = mandatory.select{ |param| options[param].nil? }
if not missing.empty?
  puts "Missing options: #{missing.join(', ')}"
  puts optparse
  exit
end

dir = options[:dir] || Dir.getwd
if (!(File.directory?(dir) && File.writable?(dir)))
  puts "Directory not valid or not writable: #{dir}"
  puts optparse
  exit
end

if ARGV.length == 1
  year_month = ARGV.first
  if !year_month.match(/\d{4}-\d{2}/)
    puts "Invalid date format: #{year_month}"
    puts optparse
    exit
  end
else
  year = Time.now.strftime("%Y")
  month = Time.now.strftime("%m")
  year_month = "#{year}-#{month}"
end

filename = "pragpub-#{year_month}.mobi"
year = year_month[0..3]

url = "http://magazines.pragprog.com/#{year}/#{filename}"
filepath = File.join(dir, filename)

if File.exists?(filepath)
  puts "#{filename} already exists"
else
  puts "Downloading #{filename}..."
  begin
    content = open(url).read
  rescue OpenURI::HTTPError => e
    abort "Unable to download #{filename}: #{e.message}"
  end
  open(filepath, 'wb') do |file|
    file << content
  end
  mail_to = options[:mailto]
  mail_from =  options[:mailfrom]
  puts "Sending #{filename} to #{mail_to}..."
  mail = Mail.new
  mail.to = mail_to
  mail.from = mail_from
  mail.subject = filename
  mail.attachments[filename] = {
    :mime_type => 'application/x-mobipocket-ebook',
    :content => File.read(filepath)
  }
  mail.deliver!
end
\$\endgroup\$
1
\$\begingroup\$

For the first snippet there is no much room for improvement, but you can use File.write:

begin
  content = open(url).read
  File.write(filepath, content)
rescue OpenURI::HTTPError => e
  abort "Unable to download #{url}: #{e.to_s}"
end

For the options parsing part, I'd definitely recommend something better than optparse, for example https://github.com/jashmenn/trollop/

\$\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.