Importing Excel file to a database just once

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

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