Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

In my Rails app I need to import some files from CSV and Excel files. I needed it in 2 models so I have written lib/importable.rb:

module Importable
  def self.included(base)
    base.send :extend, ActAsMethods
  end

  module ActAsMethods
    def act_as_importable(&block)
      @import_block = block
      send :extend, ImportableMethods
    end

    attr_reader :import_block
  end

  module ImportableMethods
    def import_from(file, *args)
      raise 'This model is not importable' if import_block.nil?

      send(:before_import, *args) if respond_to? :before_import

      sheet = open_spreadsheet(file)

      header = sheet.row(1).map(&:to_sym)

      (2..sheet.last_row).map do |i|
        row = Hash[header.zip(sheet.row(i))]

        import_block.call(row, *args)
      end
    end

    private
    def open_spreadsheet(file)
      logger.info "Importing file #{file.original_filename}"
      case File.extname(file.original_filename)
      when '.csv'  then Csv.new(file.path, nil, :ignore)
      when '.xls'  then Excel.new(file.path, nil, :ignore)
      when '.xlsx' then Excelx.new(file.path, nil, :ignore)
      when '.ods'  then Openoffice.new(file.path, nil, :ignore)
      else raise "Unknown file type: #{file.original_filename}"
      end
    rescue
      raise "Cannot open file: #{file.original_filename}"
    end
  end
end

class ActiveRecord::Base
  include Importable
end

And in model I need to write, i.e.:

class Invoice < ActiveRecord::Base
  act_as_importable do |row, company|
    invoice = company.invoices.find_or_initialize_by_name(row[:name].to_s)

    vat_in = if row[:client].is_a? String
               row[:client]
             else
               row[:client].to_i
             end
    client = company.clients.find_by_vat_in(vat_in.to_s)

    invoice.client = client

    invoice.pay_date = row[:pay_date]
    invoice.full_amount = row[:full_amount].to_i
    invoice.paid_amount = row[:paid_amount].to_i

    invoice.save!

    invoice
  end
end

The importing in controller looks like:

  def save
    file = params[:file]

    logger.debug file.path

    Invoice.transaction do
      @invoices = Invoice.import_from(file, current_company)
      @invoices.each(&:save)
    end
  rescue RuntimeError => error
    logger.debug error
    redirect_to import_company_clients_path, alert: I18n.t('invalid_file')
  rescue ActiveRecord::RecordInvalid
    redirect_to import_company_clients_path, alert: I18n.t('invalid_record')
  else
    respond_to do |format|
      format.html
      format.json { render json: @invoices }
    end
  end

But IMHO it's a little overbloated. Have you any suggedtions to make it more reagable and Gemified?

share|improve this question

1 Answer 1

Notes:

  • Your code looks pretty good, but I personally wouldn't have written it as a mixable module, this code seemps completely independent from ActiveRecord. I'd write a SpreadSheetReader class and just use it from wherever you want (this way the code is more modular).

  • Where is import_from called from? it seems some code is missing.

  • This vat_in conditional seems too verbose. Which are the possible values of row[:client]?

I'd write something like:

company = company_from_somewhere_i_dont_know
columns = [:name, :client, <other columns>...]

SpreadSheetReader.rows(:columns => columns).each do |row|
  invoice = company.invoices.find_or_initialize_by_name(row[:name].to_s)
  vat_in = row[:client].is_a?(String) ? row[:client] : row[:client].to_i.to_s
  client = company.clients.find_by_vat_in!(vat_in)
  invoice.update_attributes!({
    :pay_date => row[:pay_date],
    :full_amount => row[:full_amount].to_i,
    :paid_amount => row[:paid_amount].to_i,
  })
  invoice
end
share|improve this answer

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.