Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote this little script in PHP a bit ago. It takes a .csv file and outputs a .txt file with certain data arranged in a "psuedo-array" looking format. I recently started messing around with ruby so I decided to redo it. It wound up essentially an exact translation though, I'm wondering if there is a simpler / more ruby way to do this, and of course suggestions as to how to improve my coding in general. New to the whole code review thing. Hope it doesn't show too much.

require "csv"

class ArrayGenerator

    def initialize( file )
        @file = file
        open_csv
    end

    private
    def open_csv
        File.open("#{@file}" + "_output.txt", 'w') do |file|
            CSV.foreach(@file) do |row|
                lat = row[6]
                lon = row[7]
                img = "#{row[0]}".downcase
                part = "#{row[2]}"
                loc = "#{row[5]}"
                country = "#{row[4]}"
                focus =  "#{row[3]}"
                desc = "#{row[8]}"
                link = "#{row[9]}"

                file.write("[#{lat}, #{lon}, #{img}, #{part}, #{loc}, #{focus}, #{country}, #{desc}, #{link}],
")
            end
        end
    end
end

ARGV.each do |file|
    g = ArrayGenerator.new( file )
end

EDIT: After @sepp2k suggestions code looks like this.

require "csv"

def generate_array( file )
    File.open("#{file}" + "_output.txt", 'w') do |output|
        CSV.foreach(file) do |img, _, part, focus, country, loc, lat, lon, desc, link|
            output.puts("[#{lat}, #{lon}, '#{img.downcase}', '#{part}', '#{loc}', '#{focus}', '#{country}', '#{desc}', '#{link}'],")
        end
    end
end

ARGV.each do |file|
    generate_array(file)
end
share|improve this question

1 Answer

up vote 7 down vote accepted
img = "#{row[0]}".downcase
part = "#{row[2]}"
loc = "#{row[5]}"
country = "#{row[4]}"
focus =  "#{row[3]}"
desc = "#{row[8]}"
link = "#{row[9]}"

The only difference between writing "#{ expression }" (without anything else in the string) and just writing expression is that the former converts the result of the expression to a string. If that is your intent, you should write expression.to_s instead because that way it's clearer what's going on.

However in your case the values in row are already string, so there's no need to convert them to strings, so using "#{}" here is just redundant. You can just write img = row[0].downcase, part = row[2] etc.

Instead of taking each column out of the row array individually, you can also write this:

CSV.foreach(@file) do |img, _, part, focus, country, loc, lat, lon, desc, link|
  img = img.downcase
  file.puts("[#{lat}, #{lon}, #{img}, #{part}, #{loc}, #{focus}, #{country}, #{desc}, #{link}],")
end

Note that I've used the variable name _ to denote a variable we're not going to use because you never used row[1] in your code (_ is conventionally used in Ruby to denote variables that aren't used and is in fact the only variable name that's allowed to appear multiple times in a parameter list).

I've also replaced write with puts which automatically inserts a newline after the string, so you don't have to include the newline in the string.


A couple of more general design notes:

I think open_csv is rather misleadingly named. It does a lot more than just opening the csv file.

I also think it's a bad idea to call open_csv in your constructor. Generally the constructor should only set up the object, so that it's in a usable state. It should not do any actual work. The way you designed it, everything the object is ever going to do is done in the constructor. That's why when you use it, you create the object with ArrayGenerator.new and then never do anything with the object - because everything that can be done with the object has already been in the constructor. Another sign of this is that the only public method in your class is initialize. This is a very strange way to use objects.

On a related note there's no point in creating a g variable if you're not going to use it.

You should either redesign your class to behave more like an actual class, with a usage more like this:

g = ArrayGenerator.new( file )
g.generate

Or you could get rid of your class altogether and instead define a stand-alone method that could be used like this:

generate_array( file )
share|improve this answer
Beautiful. I will return someday with 15 rep and upvote you, sir or madam. – Michael Gruber Sep 7 '12 at 16:48

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.