How can I make this CSV Importer code better of any code smell?

Question

I wrote this method a few years ago in a Ruby on Rails project, which I think I am not proud of. How can I make this code better to show in-depth professionalism in Ruby?

lib/merchant/web_csv_importer.rb

# frozen_string_literal: true

require 'open-uri'
require 'csv'

class WebCsvImporter
  attr_accessor :url

  def initialize(url)
    @url = url
  end

  def call
    data = URI.open(url).read.force_encoding('UTF-8')

    parse_csv(data)
  end

  private

  def parse_csv(data)
    counter = 0
    duplicate_counter = 0

    CSV.parse(data, headers: true, header_converters: :symbol) do |row|
      next unless row[:name].present? && row[:email].present? && row[:status].present?

      role = Role.find_or_create_by(name: 'admin')
      merchant = Merchant.new row.to_h
      merchant.roles << role
      merchant.save

      merchant.persisted? ? counter += 1 : duplicate_counter += 1
      p "Email duplicate record: #{merchant.email} - #{merchant.errors.full_messages.join(',')}" if merchant.errors.any?
    end

    p "Imported #{counter} merchant, #{duplicate_counter} duplicate rows ain't added in total" # dont forget to pluralize this statements
  end
end

lib/tasks/merchant/csv_importer.rake

require 'merchant/web_csv_importer'

namespace :merchant do
  desc "Imports data from a Web CSV into Merchant's table"
  task web_csv_importer: :environment do
    WebCsvImporter.new('http://expample.com/merchant.csv').call
  end
end

Answer 1

I like that you already extracted a WebCsvImporter Plain Old Ruby Object which makes the code already a lot easier to understand and refactor.

Here is a refactored solution which I will explain in more detail:

class CsvImporter
  Result < Struct.new(:merchants) do
    def valid_count
      @valid_count ||= merchants.count(&:persisted?)
    end

    def invalid_count
      @invalid_count ||= total - valid_count
    end

    def total
      @total ||= merchants.count
    end

    def errors
      merchants.map do |merchant|
        merchant.errors.full_messages.to_sentence
      end.to_sentence
    end
  end

  def initalize(url)
    @url = url
  end

  def call
    merchants = valid_rows.map do |row|
      Merchant.create(
        roles: [Role.find_or_create_by(name: 'admin')],
        name: row[:name],
      )
    end

    Result.new(merchants)
  end

  private

  attr_accessor :url

  def valid_rows
    CSV.parse(data, headers: true, header_converters: :symbol).select do |row|
      row[:name].present? && row[:email].present? && row[:status].present?
    end
  end

  def data
    @data||= URI.open(url).read.force_encoding('UTF-8')
  end
end

Note that I removed the p prints and return a result object instead. You can use this result object to print the output in different formats or silence. Another solution could be to write to a logger object but you should avoid using puts as it will make testing and reusing of this class harder.

Here are a couple of suggestions:

Replace Temp with Query

Replace Temp with Query is a standard refactoring method which often improves the code quality a lot. Here we use the temp variable to a method which will also remove the need for a method parameter of parse_csv.

def data
  @data||= URI.open(url).read.force_encoding('UTF-8')
end

Extract helper methods

Extrating helper methods is also a standard refactoring method. Your parse_csv method is quite long and complicated so we should think about splitting it apart. One thing I did was extracting skipping invalid rows like this.

def valid_rows
  CSV.parse(data, headers: true, header_converters: :symbol).select do |row|
    row[:name].present? && row[:email].present? && row[:status].present?
  end
end

Don't use to_h to create object

Rather than using to_h and pass the all params to the object creation I would rather only pass in allowed parameters. I just used name as example here. It is problematic to pass all parameters in as it could easily break your application (e.g. CSV and database always need to have the same format) or worst case this could be a security issue (e.g. Merchant has an is_admin or password attribute which could get overwritten) without noticing. Specifying which params are allowed here seems like a good idea.

Result object

I return a result object and do the counting logic in the result object to make it simpler. This also has the advantage to move the printing / output to a different layer making testing a lot easier. For instance this test

def test_valid_count
  result = CsvImporter.new('example.com').call

  assert_equal(1, result.valid_count)
end

seems a lot easier and robust than

def test_valid_count
  io = capture_io do 
    CsvImporter.new('example.com').call
  end

  assert_equal("Imported 1 merchant, 1 duplicate rows ain't added in total", io)
end

Make attr_accessor private

If the attr_accessor are not accessed externally you should move them to be private.

Hope that helps, let me know what you think.

Update

Reading a little further, I am concerned about performance issues when the data I want to pull is large and save into database. An example is CSV.parse which surely will result into issues when the csv dataset is large. Though I do not know the .select you append to it will solve that issue.

Reading files generally is quite fast. If you have of course 100s of millions of rows this can become a performance bottleneck. Generally I would advise to do benchmarks with your input files to see if this is really a bottleneck rather than premature optimisations.

However, here are a few ideas what you could do:

1. Divide & Conquer: Rather than having one single CSV file you could split the file into many files and start an import worker for each of them. Ideally this would happen already when generating the CSV files because then you also distribute the download time (rather than e.g. download 1GB you download 10 * 100MB).

2. Read the CSV file line by line

Also, I think I should move row[:name].present? && row[:email].present? && row[:status].present? to a separate predicate method.

Personally I don't think this will improve much readability because you would end up with either having a method with parameter or need to extract another object which both are not ideal.

# need a method parameter
def valid_row?(row)
  row[:name].present? && 
    row[:email].present? && 
    row[:status].present?
end

# extract object
class Row
  def initialize(params)
    @params = params
  end

  def valid?
    params[:name].present? && 
      params[:email].present? && 
      params[:status].present?
  end
end

Row.new(row_params).valid?

Also what happens if those rows are not present.

This shouldn't be a problem as nil.present? will return false.