Looking to clean up a small ruby script.

Question

I'm looking for a much more idiomatic way to do the following little ruby script.

File.open("channels.xml").each do |line|
  if line.match('(mms:\/\/{1}[a-zA-Z\.\d\/\w-]+)')
    puts line.match('(mms:\/\/{1}[a-zA-Z\.\d\/\w-]+)')
  end
end

Thanks in advance for any suggestions.

Answer 1

One very easy to read approach is just to store the result of the match, then only print if there's a match:

File.open("channels.xml").each do |line|
  m = line.match('(mms:\/\/{1}[a-zA-Z\.\d\/\w-]+)')
  puts m if m
end

If you want to start getting clever (and have less-readable code), use $& which is the global variable that receives the match variable:

File.open("channels.xml").each do |line|
  puts $& if line.match('(mms:\/\/{1}[a-zA-Z\.\d\/\w-]+)')
end

Answer 2

The original:

File.open("channels.xml").each do |line|
  if line.match('(mms:\/\/{1}[a-zA-Z\.\d\/\w-]+)')
    puts line.match('(mms:\/\/{1}[a-zA-Z\.\d\/\w-]+)')
  end
end

can be changed into this:

m = nil
open("channels.xml").each do |line|
  puts m if m = line.match(%r|(mms://{1}[\w\./-]+)|)
end

File.open can be changed to just open.

if XYZ
   puts XYZ
end

can be changed to puts x if x = XYZ as long as x has occurred at some place in the current scope before the if statement.

The Regexp '(mms:\/\/{1}[a-zA-Z\.\d\/\w-]+)' can be refactored a little bit. Using the %rXX notation, you can create regular expressions without the need for so many backslashes, where X is any matching character, such as ( and ) or in the example above, | |.

This character class [a-zA-Z\.\d\/\w-] (read: A to Z, case insensitive, the period character, 0 to 9, a forward slash, any word character, or a dash) is a little redundant. \w denotes "word characters", i.e. A-Za-z0-9 and underscore. Since you specify \w as a positive match, A-Za-z and \d are redundant.

Using those 2 cleanups, the Regexp can be changed into this: %r|(mms://{1}[\w\./-]+)|

If you'd like to avoid the weird m = nil scoping sorcery, this will also work, but is less idiomatic:

open("channels.xml").each do |line|
  m = line.match(%r|(mms://{1}[\w\./-]+)|) and puts m
end

or the longer, but more readable version:

open("channels.xml").each do |line|
  if m = line.match(%r|(mms://{1}[\w\./-]+)|)
    puts m
  end
end

Answer 3

Personally, I would probably just use the POSIX grep command. But there is Enumerable#grep in Ruby, too:

puts File.readlines('channels.xml').grep(%r|mms://{1}[\w\./-]+|)

Alternatively, you could use some of Ruby's file and line processing magic that it inherited from Perl. If you pass the -p flag to the Ruby interpreter, it will assume that the script you pass in is wrapped with while gets; ...; end and at the end of each loop it will print the current line. You can then use the $_ special variable to access the current line and use the next keyword to skip iteration of the loop if you don't want the line printed:

ruby -pe 'next unless $_ =~ %r|mms://{1}[\w\./-]+|' channels.xml

Basically,

ruby -pe 'next unless $_ =~ /re/' file

is equivalent to

grep -E re file