Some notes:

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

• First line is too long. < 80/100 chars is advisable. When writing hashes, it's easy: one line per pair.

• # if existed then return or continue the remaining: This comment is repeating the code, so not really useful.

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

• Answering your question about the if, two options:

  • One-line guard: return false if ImportHistory.where(q_param).exists?
  • Write all the rest of the code inside 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

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

Some notes:

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

• First line is too long. < 80/100 is advisable.

• # if existed then return or continue the remaining: This comment is repeating exactly what the code says it's doing. Not useful.

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

• Answering your question about the if, two options:

  • return false if ImportHistory.where(q_param).exists?
  • Write all the rest of the code inside the else.

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

Some notes:

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

• First line is too long. < 80/100 chars is advisable. When writing hashes, it's easy: one line per pair.

• # if existed then return or continue the remaining: This comment is repeating the code, so not really useful.

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

• Answering your question about the if, two options:

  • One-line guard: return false if ImportHistory.where(q_param).exists?
  • Write all the rest of the code inside 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