8
\$\begingroup\$

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 as an exact translation, though.

Is there a simpler / more Ruby way to do this?

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
\$\endgroup\$
10
\$\begingroup\$
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 )
\$\endgroup\$

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.