1
\$\begingroup\$

I have written the following code that achieves my desired goal of converting a "raw" CSV file into a keycoded list. The idea is that a person will get the first product they ask for, and English products are coded as blank because they are the default.

This gist contains a sample of the input and output data. As it stands, rubocop thinks my code is too complex, and I am inclined to agree with it here, as it feels like I am doing far more than I need to in order to get such a simple result.

Not sure if this is helpful but the rubocop stats I'm getting are as follows:

  • ABC size too high (19.44/15)
  • Cyclomatic complexity too high (9/6)
  • Method has too many lines (17/10) (is this one really even a problem?)
  • Perceived complexity too high (10/7)

Are any of these "true" problems? They seem to reflect my opinion that the code just turned out "ugly"

On paper it I narrowed it down to about 2 cases with some sub cases. (1. Normal header -> write 2.Header has "#" in it -> subcases) and in the second case it's either blank, "1", or a product name. If the line contains "English Map" I write nothing, otherwise I am to write the first product requested sequentially on the line.

require 'CSV'
in_file = 'test.csv'

def code_line_by_product(line)
  new_line = []
  tmp = nil
  e_line = false
  line.each do |item|
    if item[0].include?('#')
      if item[1] != ' ' && item[1] && item[1] != '1' && item[1] != ''
        e_line = true if item[1].include?('English')
        unless tmp || e_line
          tmp = item[1][0]
          new_line << tmp
        end
      end
    else
      new_line << item[1]
    end
  end
  new_line
end

CSV.open("#{File.basename(in_file, '.csv')}_coded.csv", 'a+') do |out|
    out << %w(First Last Company Address address2 City State Zip Country phone_number email_address keycode)
  CSV.foreach(in_file, headers: true) do |line|
    out << code_line_by_product(line)
  end
end

Also, I've been noticing that I use that construction of writing from one file to another a lot and was wondering if there was a more convenient way of doing it (such as writing a function that takes the in file, out file, and the desired transformation). While it's only 4 lines of code, I essentially use those nested block all the time and it would be nice to only have to write the transformation function and then just call a single function to execute it.

\$\endgroup\$
  • 2
    \$\begingroup\$ Unrelated to the actual code: Are those actual names and email addresses in the CSV? Because if it is, you'll want to remove it as fast as you possibly can! \$\endgroup\$ – Flambino Apr 22 '15 at 11:28
  • \$\begingroup\$ Thank you for pointing that out! I John Smithed it. That could have turned into a very bad habit. \$\endgroup\$ – Soviet_Jesus Apr 22 '15 at 13:19
  • 1
    \$\begingroup\$ Great, but: gists are just git commits, so the original data are still there (click "Revisions"). I can't remember if there's an option to remove the gist revisions, or if it's easier to simply make a new gist. Either way, you should look into it. \$\endgroup\$ – Flambino Apr 22 '15 at 14:36
  • 1
    \$\begingroup\$ Better. And yeah, just don't mess with user data. Ever, if possible. E.g. write some scripts to generate data for you (you'll end up doing that anyway if you write tests), or at least sanitize/anonymize real data before going near it. The latter is more dangerous, since stuff can slip through, or simply because there's no way to truly make it anonymous (cf. even metadata becomes actual data if you've got enough of it). Basically, handle user data like an epidemiologist handles a horrible virus: Wear tons of protective gear, have strict procedures, and don't let it out of the lab. \$\endgroup\$ – Flambino Apr 22 '15 at 18:41
  • 1
    \$\begingroup\$ The main point is just to avoid exposing other people's data, so the more you do to prevent that, the less likely it is. As for testing, certainly look into that! Ruby coders are big on testing, so there's lots to find; Test::Unit is built in (stdlib), but there are many more tools and libraries. So it shouldn't be too hard to, for instance, write a few tests for your current code_line_by_product that verifies that its output is correct. Still, the final test is of course the real data and your eyes. Just don't post the results online :) \$\endgroup\$ – Flambino Apr 22 '15 at 21:42
1
\$\begingroup\$

I would do something like this. It is untested and may have typos but it should give you some idea of how to break the logic up. I would:

  • Express variable to what they mean
  • Use a class

Hope this helps

require 'CSV'

class CSVTranslator
  def self.write file_path, options={}
    new(file_path, options).translate
  end

  def initialize source_file, options={}
    @source_file = source_file
    @destination_file = options.fetch(:to) { "#{File.basename(source_file, '.csv')}_coded.csv" }
  end

  def translate
    CSV.open(destination_file, 'a+') do |out|
      out << headers
      each_row { |row| out << row }
    end
  end

  private

  attr_reader :source_file, :destination_file

  def headers
    %w(First Last Company Address address2 City State Zip Country phone_number email_address keycode)
  end

  def each_row 
    CSV.foreach(source_file, headers: true) do |line|
      yield code_line_by_product(line)
    end
  end

  def code_line_by_product(line)
    new_line = {}
    english_language = false
    keycode = ""

    line.each do |header, value|
      if header.include?('#')
        english_language ||= value.include?('English') 
        keycode = value.first if value.first != '1'
      else
        new_line[header] = value
      end
    end

    new_line['keycode'] = keycode unless english_language || keycode.blank?

    new_line
  end
end

CSVTranslator.write 'test.csv', to: 'test_coded.csv'
\$\endgroup\$
  • \$\begingroup\$ So if I'm getting the idea here, I was getting too caught up in mucking about with arrays when a hash could have handled a lot of the heavy lifting for me? \$\endgroup\$ – Soviet_Jesus Apr 22 '15 at 18:19
  • \$\begingroup\$ I still think there is a better way by being explicit in the headers. Will the source csv headers vary? \$\endgroup\$ – natedavisolds Apr 22 '15 at 18:25
  • \$\begingroup\$ In this particular case I don't believe they will. Ideally I would just take the original headers, remove the ones containing "#" and then tack on the keycode header. That part where I'm explicitly creating the headers was kinda a lazy solution to what I'm now realizing could probably be done using the array deleteif method or whatever. \$\endgroup\$ – Soviet_Jesus Apr 22 '15 at 18:39

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.