File Renaming with Ruby

Question

This is a minimal little utility that I've found useful while reorganizing my music and book libraries. Any issues you can see? Anything I could be doing better or more elegantly? Anyone wanna take a crack at the equivalent in Python/Perl?

Take 2:

#!/usr/bin/ruby

require 'optparse'
require 'pp'
require 'fileutils'

$options = {:sub => "", :downcase => nil}
OptionParser.new do |opts|
  opts.on('-r', '--regex REGEX', String, 
          'Specify the regular expression to replace') {|reg| $options[:regex] = Regexp.new(reg)}
  opts.on('-s', '--sub SUBSTITUTE', String, 
          'Specify what to replace the match with. By default, the empty string (so matches are stripped).') {|$options[:sub]|}
  opts.on('-d', '--downcase', 'If passed, all filenames will be downcased.'){|$options[:downcase]|}
end.parse!

usage unless ARGV.length > 0

def rename(str)
  ($options[:downcase] ? 
   str.downcase : str).gsub($options[:regex], $options[:sub])
end

ARGV.each do |target|
  File.rename(target, rename(target))
end

Answer 1

Some observations. The space between #! and the path to binary is not needed. (It used to be for some arcane versions of Unix but not anymore AFAIK)

Secondly, I prefer to let env find ruby for portability.

#!/usr/bin/env ruby

require 'optparse'
require 'pp'
require 'fileutils'

The options are globals. So perhaps it is better to specify that explicitly. This will help in modularization later.

$options = {:sub => "", :downcase => nil}

In the interests of people who have to use 80 char width terminals, I advice an 80 cols limit (or near abouts) for lines :) . I would also advice on verbose and dryrun flags to tell the user what is going to happen, and what is happening.

The stdlib example is to just call .parse! directly and I think that that style is more rubyish than creating a new variable optparse.

OptionParser.new do |opts|
  opts.on('-r', '--regex REGEX', String, 
     'Specify the regular expression to replace'){|$options[:regex]|}
  opts.on('-s', '--sub SUBSTITUTE', String, 
     'Specify what to replace the match with. By default, the \
      empty string (so the matched patterns are \
      stripped).'){|$options[:sub]|}
  opts.on('-d', '--downcase', 'If passed, all filenames will be \
      downcased.'){|$options[:downcase]|}
end.parse!

I like programs to give me the usage if they are invoked with no input values.

usage unless ARGV.length > 0

I also think that the transformation of old name to new name should be a separate method. And why embed a string inside a string and then transform to a regular expression when you can directly transform that string to a regexp?

def transform(str)
  ($options[:downcase] ? 
      str.downcase : str).gsub(Regexp.new $options[:regex], $options[:sub])
end

And given all these, I prefer a little more functional way of processing The advantage is that you are restricting the IO to a very small portion.

ARGV.map{|target| [target, transform(target)]}.each do |old,new|
  File.rename(old, new)
end

Now, these comments are highly subjective. But given the length of your snippet, I suppose that is justified? Because it obviously works as given.