10
\$\begingroup\$

I wrote the following Ruby script several years ago and have been using it often ever since.

It opens a text editor with the list of files in current directory. You can then edit the file names as text. Once you save and exit the files are renamed.

The renamer allowed me to maintain spreadsheet-like file names thanks to the powerful column editing capabilities of vi (emacs also has those).

Here is how one such directory looks:

enter image description here

I would like to use it as an example for a Ruby workshop. Could you please suggest best-practice and design pattern improvements?

#!/usr/bin/ruby

RM = '/bin/rm'
MV = '/bin/mv'

from = Dir.entries('.').sort; from.delete('.'); from.delete('..')
from.sort!

from.delete_if {|i| i =~ /^\./} # Hidden files

tmp = "/tmp/renamer.#{Time.now.to_i}.#{(rand * 1000).to_i}"

File.open(tmp, 'w') do |f|
  from.each {|i| f.puts i}
end

ENV['EDITOR'] = 'vi' if ENV['EDITOR'].nil?
system("#{ENV['EDITOR']} #{tmp}")

to = File.open(tmp) {|f| f.readlines.collect{|l| l.chomp}}
`#{RM} #{tmp}`

if to.size != from.size
  STDERR.puts "renamer: ERROR: number of lines changed"
  exit(1)
end

from.each_with_index do |f, i|
  puts `#{MV} -v --interactive "#{f}" "#{to[i]}"` unless f == to[i]
end
\$\endgroup\$
9
\$\begingroup\$
RM = '/bin/rm'
MV = '/bin/mv'

In general it's preferable to use the FileUtils class rather than relying on shell utilities. Though in this particular case, you might want to stick at least with mv since the FileUtils.mv method does not have an :interactive option.


tmp = "/tmp/renamer.#{Time.now.to_i}.#{(rand * 1000).to_i}"

Ruby has a Tempfile class which can generate a unique temporary file more reliably than this. You should use it.


from.each {|i| f.puts i}

Calling puts on an array will puts each line individually, so the above can just be shortened to f.puts from.


ENV['EDITOR'] = 'vi' if ENV['EDITOR'].nil?

Can be shortened to ENV['EDITOR'] ||= 'vi'. Though what you have isn't particularly verbose either, so it doesn't really matter much which one you choose.


system("#{ENV['EDITOR']} #{tmp}")

Use system(ENV[EDITOR], tmp) instead. This way you get rid of the string interpolation and the code still works if either ENV['EDITOR'] or tmp should contain a space or other shell meta-character (not that they're particularly likely to, but it's a good idea to use the multiple-argument-form of system where ever possible).


to = File.open(tmp) {|f| f.readlines.collect{|l| l.chomp}}

Usually this could be replaced with to = File.readlines(tmp).collect {|l| l.chomp}. However if you follow my suggestion of using Tempfile, that won't be an option any more.


`#{RM} #{tmp}`

If you use fileutils, this will just be FileUtils.rm(tmp) (or rm(tmp) if you include FileUtils). If you don't want to use FileUtils, you should at least use system(RM, tmp) for the same reasons as above.

However if you use Tempfile, which you should, this becomes redundant anyway.


from.each_with_index do |f, i|
  puts `#{MV} -v --interactive "#{f}" "#{to[i]}"` unless f == to[i]
end

To iterate over two arrays in parallel, use zip:

from.zip(to) do |f, t|
  system(MV, "-v", "--interactive", f, t) unless f == t
end

Note that here using system instead of backticks is especially important since one of the files in from or to containing spaces is actually somewhat likely.


So with all my suggestions, your code should now look like this:

#!/usr/bin/env ruby
require 'tempfile'

MV = '/bin/mv'

from = Dir.glob('*').sort

ENV['EDITOR'] ||= 'vi'

to = nil
Tempfile.open("renamer") do |f|
  f.puts from
  f.close
  system(ENV['EDITOR'], f.path)
  f.open
  to = f.readlines.collect {|l| l.chomp}
end

if to.size != from.size
  STDERR.puts "renamer: ERROR: number of lines changed"
  exit(1)
end

from.zip(to) do |f, t|
  system(MV, "-v", "--interactive", f, t) unless f == t
end
\$\endgroup\$
  • \$\begingroup\$ Surprisingly, to = Tempfile.open(...){...; "result"} does not work. to gets nil. Ruby 1.8.7 \$\endgroup\$ – Aleksandr Levchuk Feb 6 '11 at 21:33
  • \$\begingroup\$ @Aleksandr: Bah, stupid of me to forget to replace the tmp. That open does not return the block's result surprises me too (I had not tested it previously). Anyway I've fixed it. \$\endgroup\$ – sepp2k Feb 6 '11 at 21:39
  • \$\begingroup\$ A very good review of the code and a good look at "idiomatic" Ruby. Well done \$\endgroup\$ – Michelle Tilley Feb 9 '11 at 1:32
5
\$\begingroup\$

Just a couple of things:

#!/usr/bin/ruby

It's possible that on some systems this isn't where ruby lives, it's better to do:

#!/usr/bin/env ruby

from = Dir.entries('.').sort; from.delete('.'); from.delete('..')

Can be written as

from = Dir.entries('.').sort - ['.', '..']

Which is more succinct and eliminates having three statements on one line (which you shouldn't do).

Or you could eliminate hidden files and . / .. in one go with:

from = Dir.entries(.).select do |filename|
    filename[0].chr != '.'
end

Edit:

from = Dir.glob("*").sort

Is definitely the best solution.

\$\endgroup\$
  • \$\begingroup\$ Using the /usr/bin/env trick is especially important for people who've compiled Ruby themselves or (like me, and a lot of the Ruby world) are using RVM. \$\endgroup\$ – anonymous coward Feb 6 '11 at 7:53
  • 3
    \$\begingroup\$ I just replaced the whole thing with from = Dir.glob("*").sort \$\endgroup\$ – Aleksandr Levchuk Feb 6 '11 at 8:06

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.