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
1 Answer 1
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/