3
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ I wonder if it might be better for you to add to the question rather than update it based on the review? Here is a meta discussion. \$\endgroup\$ – rahul May 29 '12 at 18:10
  • \$\begingroup\$ @blufox - I'm of the opinion that having a working diff is better than having a very tall question with multiple code blocks. Any consensus on that discussion by the way? The question you link was closed before anyone answered either way. \$\endgroup\$ – Inaimathi May 29 '12 at 18:14
  • \$\begingroup\$ See the second answer in the question linked as a duplicate of. I agree that the more reviews there are, the longer the question becomes, and perhaps reduces in understandability. Perhaps the site should provide some mechanism to fold the older revisions? Can we ask for it in meta? What do you think? \$\endgroup\$ – rahul May 29 '12 at 18:22
  • \$\begingroup\$ @blufox - There should be an "edited" link on the question which essentially provides what was proposed in that answer. Except with change highlighting (which is why I think this is the better approach). The only annoyance (which happens either way) is that responders need to note which edit they are responding to, since there's no mechanism to associate an answer with a particular version of the question. A solution may be worth asking for. \$\endgroup\$ – Inaimathi May 29 '12 at 18:49
  • \$\begingroup\$ Do you think a note saying (Original here) would help? Atleast in this case? \$\endgroup\$ – rahul May 29 '12 at 20:56
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Took most of your advice. I'm not sure about that use of the ternary operator, but it does the job. ARGV.map{|target| [target, transform(target)]}.each do |old,new|... is this really a commonly accepted idiom in the Ruby world? Is there a particular reason you wouldn't just do File.rename(target, transform(target)) instead? \$\endgroup\$ – Inaimathi May 26 '12 at 0:04
  • \$\begingroup\$ As I mentioned, it is very subjective. I feel that all IO should be isolated, and the above kind of reflects that. It is even more clear if you were doing python list comprehensions or Haskell. But in ruby it is perhaps debatable. \$\endgroup\$ – rahul May 26 '12 at 0:28

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.