Image batch processing in Ruby

Question

I know a lot about Java, PHP and stuff, but I'm fairly new to ruby.

I've written this script to look into the current working directory, get two parameters from the command line and then resize and rename all jpegs it can find.

To all rubyists, what could I have done better to make things ruby-ish?

require "rmagick"
require "readline"

CWD = Dir.pwd
MAX_DIM = ARGV[0]
PREFIX = ARGV[1]

puts "cwd: #{CWD}"
puts "max dimension: #{MAX_DIM}"
puts "prefix: #{PREFIX}"

read = Readline.readline "continue Y/n: "

exit if read.start_with? "n"

i = 1
Dir.foreach(CWD) do |file|
    next if file == "." or file == ".."
    next if !file.end_with? "jpg" and !file.end_with? "JPG"

    img = Magick::Image.read(file).first
    resized = img.resize_to_fit MAX_DIM
    newpath = CWD + "/" + PREFIX.sub("$", i.to_s) + ".jpg"

    resized.write(newpath) do
        self.quality = 80
    end

    File.delete file

    i = i + 1
    img.destroy!
    resized.destroy!
end

Answer 1

I would replace

i = 1
Dir.foreach(CWD) do |file|
    next if file == "." or file == ".."
    next if !file.end_with? "jpg" and !file.end_with? "JPG"

with

Dir[File.join(CWD, '*.png')].each_with_index do |file, i|

or

Dir.glob(File.join(CWD, '*.png')).each_with_index do |file, i|

• I prefer DIR[] over Dir.glob, but I think this is a question on your private coding style.
• File.join(CWD, '*.png')] creates a path. Advantage: the system may decide, it if use / or \ (or whatever the OS needs).

Another change I would do: Use the string format options instead of replacing $. If you want to use the $ in the command line, then you can replace the $ with a format string. I use in my example below $02i - this means, replace the string with the given integer value, use at least 2 digits and add leading zeros. This may be a help if you have more then 10 images.

Some more changes I would do:

• I wouldn't use constants to fill it with parameters from the command line. Either use global variables or normal variables.
• Instead of Dir.pwd, I would use '.'
• I would separate the real action and the user interface.
• You have no checks, if the parameters are empty. In my later example I define defaults.
• I would't check for n, but for a Yes.

My complete Code:

require "rmagick"
require "readline"

def convert_images(prefix, max_dim, path = Dir.pwd)
  #Use a glob instead of checking the pathes later,
  #Use each_with_index (Attention, i will start with 0!)
  Dir.glob(File.join(path, '*.jpg')).each_with_index do |file, i|
      img = Magick::Image.read(file).first
      resized = img.resize_to_fit(max_dim)  
      #Don't use string operations for pathes
      newpath = File.join(path,prefix % [i+1] + ".jpg") #i starts with 0, so add 1.

      resized.write(newpath) do
          self.quality = 80
      end

      puts "%s -> %s" % [ file, newpath ]
      #~ File.delete file

      img.destroy!
      resized.destroy!
    end
  end

#Define parameters with defaults.
max_dim = ARGV[0] || 100
prefix = ARGV[1]     || 'my_pic_version_%i'
prefix.sub!(/\$/, '%02i') #use string format options.

puts "max dimension: #{max_dim}"
puts "prefix: #{prefix}"

read = Readline.readline "continue Y/n: "
if read =~ /^[Yy]/
  convert_images(prefix, max_dim)
end

Some other ideas:

• Use the optparse-gem for interfaces
• prefix could contain *.jpg (and the call it target_template or similar).

Answer 2

Some notes:

• MAX_DIM: Uppercase for constants, variables should be downcase.
• MAX_DIM = ARGV[0]. Some control of the arguments length is lacking.
• read.start_with? "n". In Ruby you can omit the parens, but in "normal" code, it's not usual.
• Why use CWD? All paths are relatives anyway.
• Dir.foreach. I'd use Dir.glob
• i = 1 Use each_with_index or with_index to have loops with an index.
• img.destroy! Is this necessary? Usually you leave that to the garbage collector.
• PREFIX.sub("$", i.to_s). Does this work

I'd write:

require 'rmagick'

abort('Usage: process MAX_DIM PREFIX') unless ARGV.size == 2
max_dim, prefix = ARGV 

Dir.glob('*.{jpg,JPG}').each.with_index(1) do |path, idx|
  img = Magick::Image.read(path).first
  img_resized = img.resize_to_fit(max_dim)
  new_path = prefix.sub("$", idx.to_s) + ".jpg"
  img_resized.write(new_path) { self.quality = 80 }
  File.delete(path)
end