Importing Excel file to a database just once

Question

How should I rewrite an if-return-else condition in the middle of this function?

def import_excel_to_database(xls_file_path)
 q_param = { report_type:report_type, item:product, exchange:exchange, product_exchange:row[0], date: row[2]} # timestamp_utc: row[2].strftime('%Q').to_i
 # if exsisted then return or continue the remaining
 if ImportHistory.where(q_param).exists?
 return false
 else
 ImportHistory.create(q_param)
 end
 xls = Roo::Excel.new(xls_file_path)
 xls.default_sheet = xls.sheets[0]
 columns = get_columns(xls.row(1))
 iterate_data_rows(xls_file_path, xls, columns)
end

Answer 1

Some notes:

• AFAIK there is no consensus whether to write spaces or not after { and before }. But, definitely, always put an space after :.

• This first line is too long. < 80/100 chars is more advisable. When writing hashes, it's easy: one line per pair. Much easier to read.

• # if existed then return or continue the remaining: This comment is repeating the code, so it's not really conveying much.

• Questions: Where does row come from? what does iterate_data_rows return?

• Answering your question about the if, you have two options (you wrote the intermediate, which is not advisable):

  • One-line guard: return false if ImportHistory.where(q_param).exists?
  • Move the rest of the code within the else (I like this one).

I'd write:

def import_excel_to_database(xls_file_path)
 q_param = {
 report_type: report_type, 
 item: product, 
 exchange: exchange, 
 product_exchange: row[0], 
 date: row[2],
 # timestamp_utc: row[2].strftime('%Q').to_i,
 } 
 if ImportHistory.where(q_param).exists?
 false
 else
 ImportHistory.create(q_param)
 xls = Roo::Excel.new(xls_file_path)
 xls.default_sheet = xls.sheets[0]
 columns = get_columns(xls.row(1))
 iterate_data_rows(xls_file_path, xls, columns)
 end
end