Return to Answer

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

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).map 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

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 to 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

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

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 to verbose. Which are the possible values of row[:client]?

I'd write something like:

company = company_from_somewhere_i_dont_know
SpreadSheetReader.rows(:name, :client, [other 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

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 to 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