CSV module in Ruby

Question

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.

Given: a CSV file structured with a first line with headers and subsequent rows with data.

one, two
lions, tigers

Create a module which loads the header and values from a CSV file, based on the name of the implementing class. (RubyCSV -> "rubycsv.txt") Support an each method which returns a CsvRow object. Use method_missing to return the value for the column for a given heading. E.g. usage which will print "lions":

m = RubyCsv.new
m.each { |row| p row.one }

My implementation:

class CsvRow
 attr :row_hash
 def initialize( row_hash )
 @row_hash = row_hash
 end
 def method_missing( name, *args )
 @row_hash[ name.to_s ]
 end
end
module ActsAsCsv
 attr_accessor :headers, :csv_contents
 def self.included( base )
 base.extend ClassMethods
 end
 module ClassMethods
 def acts_as_csv
 include InstanceMethods
 end
 end
 module InstanceMethods
 def read
 @csv_contents = []
 filename = self.class.to_s.downcase + '.txt'
 file = File.new( filename )
 @headers = file.gets.chomp.split( ', ' )
 file.each do |row|
 @csv_contents << row.chomp.split( ', ' )
 end
 end 
 def initialize
 read
 end
 def each 
 @csv_contents.each do |content| 
 hash = {}
 @headers.zip( content ).each { |i| hash[ i[0] ] = i[1] }
 yield CsvRow.new hash
 end
 end
 end
end
class RubyCsv # No inheritance! You can mix it in.
 include ActsAsCsv
 acts_as_csv
end

Answer 1

def method_missing( name, *args )
 @row_hash[ name.to_s ]
end

Doing it like this means that if the user calls a row method with arguments, the arguments will silently be ignored. Also if the user calls any method which does not exist and for which no row exists either, he'll just get back nil. I think in both cases an exception should be thrown, so I'd implement method_missing like this:

def method_missing( name, *args )
 if @row_hash.has_key?(name.to_s)
 if args.empty?
 @row_hash[ name.to_s ]
 else
 raise ArgumentError, "wrong number of arguments(#{ args.size } for 0)"
 end
 else
 super
 end
end

---

module ActsAsCsv
 # ...
 def self.included( base )
 base.extend ClassMethods
 end
 module ClassMethods
 def acts_as_csv
 include InstanceMethods
 end
 end
 module InstanceMethods
 ...
 end
end

This setup seems needlessly complicated. Since all acts_as_csv does is include the instance methods (except headers and csv_contents, which will be present even if acts_as_csv is not called - which seems a bit arbitrary to me) and there is no reason why a user would want to include ActsAsCsv without getting the instance methods, I see no reason for acts_as_csv to exist at all. The instance methods should be directly in the ActsAsCsv module and the ClassMethods and InstanceMethods modules should not exist.

This way the code will be less complex, and you only need one line include ActsAsCsv instead of two to enable the CSV functionality.

---

def read
 @csv_contents = []
 filename = self.class.to_s.downcase + '.txt'
 file = File.new( filename )
 @headers = file.gets.chomp.split( ', ' )
 file.each do |row|
 @csv_contents << row.chomp.split( ', ' )
 end
end

First of all you're opening a file and never closing it. You should use File.open with a block instead.

Second of all you should make use of higher order functions like map. Using map you can create @csv_contents like this instead of appending to it in an each loop:

def read
 filename = self.class.to_s.downcase + '.txt'
 file = File.open( filename ) do |file|
 @headers = file.gets.chomp.split( ', ' )
 @csv_contents = file.map {|row| row.chomp.split( ', ' )}
 end
end

---

That being said I don't think it's a good idea to read the whole file into memory in advance (or at all), as that will make your library unusable on big files (which might not even fit into memory).

So I would get rid of the read method and just open and read through the file in the each method, like this:

def each
 filename = self.class.to_s.downcase + '.txt'
 File.open(filename) do |file|
 headers = file.gets.chomp.split( ', ' )
 file.each do |content| 
 hash = {}
 headers.zip( content.chomp.split(', ') ).each { |i| hash[ i[0] ] = i[1] }
 yield CsvRow.new hash
 end
 end
end

---

Lastly, instead of

hash = {}
headers.zip( content ).each { |i| hash[ i[0] ] = i[1] }

You can also write hash = Hash[ headers.zip( content ) ].

---

On a more general note, your code does not really correctly parse CSV files. Your code assumes that the fields will be separated by a comma followed by a single space. However it is not required that commas are actually followed by any spaces (and the RFC actually says that any space after a comma is part of the field's content and should not be ignored). You're also not handling quoting (e.g. foo, "bar, baz", bay, which is a row containing three, not four, fields) at all.

• Steven Jeuris: Actually this question is an adaptation of an example in the book. The each and CsvRow needed to be added. Now that I heard your explanation I find it a bad example, or ill explained. Others apparently thought the same. I'm definitely going to attempt some more metaprogramming in Ruby to see whether I can grasp its added value. I just need to come up with a nice relatively small exercise. :)
• Steven Jeuris: Found the authors explanation. "This was another case of editing an exercise getting the best of me. I wanted to show how Rails acts-as modules work, and initially I had some dynamic capabilities that did some introspection. But in the end, I simplified the application and removed the very capabilities (faster-csv style attribute inspection) that made acts-as a good idea. I’ll address it in the second edition."
• Steven Jeuris: Thanks for all the pointers! They've been really useful. I fixed some minor mistakes in your updated code samples, but your explanation was great. ;p
• sepp2k: @Steven: Yeah, I wasn't thinking with the each instead of map ;-) Btw: I've added a paragraph at the end.