8
\$\begingroup\$

I finished the Ruby chapter in Seven Languages in Seven Weeks. It tries to make you familiar with the core concepts of several languages rather quickly. Dutifully I did all exercises, but most likely they can be improved to be more Ruby-like.

Write a simple grep that will print the lines of a file having any occurrences of a phrase anywhere in that line (in my example 'bleh'). Include line numbers.

File.open( 'grepFile.txt', 'r' ) do |file|
  lines = []
  line_number = 1
  file.readlines.each do |line|
    lines[line_number] = line
    line_number += 1
  end
  lines.each_index do |i|  
    line = lines[i]
    puts "line #{i.to_s}: #{line}" if line =~ /bleh/
  end
end
\$\endgroup\$
7
\$\begingroup\$
File.open( 'grepFile.txt', 'r' ) do |file|
  lines = []
  line_number = 1
  file.readlines.each do |line|
    lines[line_number] = line
    line_number += 1
  end
  lines.each_index do |i|  
    line = lines[i]
    puts "line #{i.to_s}: #{line}" if line =~ /bleh/
  end
end

file.readlines.each should be file.each_line, which will do the same thing (iterate over all the lines) without building an array (and reading the whole file into memory) first. Also instead of keeping track of the line_number manually, you should just use each_with_index. Though actually if you want to append to the end of the array, you actually shouldn't use an index at all, but use << or push instead (if you want the indices to start at 1, you can just insert a nil element first).

That being said if all you want is to read the file into an array, you could just do lines = file.readlines without any loop (or possible [nil] + file.readlines, so that it keeps starting at 1, but I'd rather add 1 to the line number when printing instead of copying the whole array).

Further you can use the methods File.readlines or File.foreach instead of File.open + File#readlines or File.open + File#each_line respectively.

Then when you iterate over lines you should use each_with_index rather than each_index, so you don't have to do lines[i] to get at the value.

As a last note, it's unnecessary to call to_s on an object in #{} - ruby does that automatically.


However the whole approach seems needlessly complicated to me. I don't see why you need to build up an array at all. I'd just iterate over the lines once and output them directly. This in addition to being much shorter also has the advantage, that the program runs in O(1) space (assuming the maximum line length is constant) rather than reading the whole file into memory.

File.foreach('grepFile.txt').each_with_index do |line, i|
  puts "line #{i}: #{line}" if line =~ /bleh/
end

And of course instead of 'grepFile.txt' and /bleh/, you should be using ARGV[1] and Regexp.new(ARGV[0]), as it makes no sense to hardcode these values.

\$\endgroup\$
  • 1
    \$\begingroup\$ Nice! I was looking for a way to also get the index, but was looking in the wrong place (an overload of each which now I know isn't really supported). The whole reason for the array was to store the line number. ;) Once I read your pointers, I quickly came to the last code sample myself. I just have #{i+1} for the line number. \$\endgroup\$ – Steven Jeuris Apr 28 '11 at 14:59
  • 1
    \$\begingroup\$ The last bit of code really should be at the top. That's ruby-way structure, and the OPs initial code is just due to their early learning. \$\endgroup\$ – New Alexandria Oct 16 '12 at 15:19

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.