1
\$\begingroup\$

Here is the pseudo code used inside a Rails controller:

defined_hash_10000_records.each do |data|
    account = Account.where(account_identifier: params[:account_id]).first
    program = Program.where(program_identifier: data[:id]).first
    unless participant_account.present? || utility_program.present?
      # this is just a logger method defined in the second file.
      failure_msg('participant account is #{account.inspect} & utility program is #{program.inspect}')
      next
    end
    sub = Subscription.where(account_id: account.id, program_id: program.id)
end

It is working fine and I would like to know if it can be further improved.

Focus to improve the following line (conditions will help to bypass in case record not found instead of break the execution):

unless participant_account.present? || utility_program.present?
      # this is just a logger method defined in the second file.
      failure_msg('participant account is #{account.inspect} & utility program is #{program.inspect}')
      next
    end
\$\endgroup\$
2
  • \$\begingroup\$ What do you want to improve? speed? memory? readability? \$\endgroup\$
    – Feras
    Commented Nov 3, 2016 at 17:54
  • \$\begingroup\$ readability and memory \$\endgroup\$ Commented Nov 4, 2016 at 7:40

1 Answer 1

1
\$\begingroup\$

Simplify logic to eliminate next

To improve readability by eliminating next, rearrange your record validity test, so lines 4 to 9 become

if participant_account.present? || utility_program.present?
  sub = Subscription.where(account_id: account.id, program_id: program.id)
else
  # this is just a logger method defined in the second file.
  failure_msg('participant account is #{account.inspect} & utility program is #{program.inspect}')
end

I am not sure this is what you mean. Ask yourself if you really need only one of participant_account or utility_program for the subscription query to succeed.

Don't do things many times that could be done once

To save some time, put line two before the iteration, because value of account does not depend on data. This also helps readability and debugging, as when programmers read the body of an iteration, they expect only to see lines that need to be evaluated many times.

Only make queries if you need the result

To save a little more time and memory, only calculate the value of program if you know you are going to use it. So put it inside the conditional:

if participant_account.present? && utility_program.present?
  program = Program.where(program_identifier: data[:id]).first
  sub = Subscription.where(account_id: account.id, program_id: program.id)
else # ...

Extract methods

After these changes the conditional is starting to get a bit complicated, so extract some methods, for example:

if participant_account.present? && utility_program.present?
  sub = subscription(account, first_program(data))
else # ...

def subscription(account, program)
  Subscription.where(account_id: account.id, program_id: program.id)
end

def first_program(data)
  Program.where(program_identifier: data[:id]).first
end

Put query logic in models

I don't know if Program is an ActiveRecord model. If it is, then #first_program could be a class method in that model

Don't use #first unless you guarantee the order

For example, if you have ActiveRecord timestamps, you could use

Program.where(program_identifier: data[:id]).order(:created_at).first

Are validation and failure handling cross-cutting concerns?

You might also want to consider patterns that factor out your validation and failure handling code, but whether and how to use them depends on the larger structure of your program. For example, you could yield to your validations and sub = ... line as lambdas from a validate_with_logging method.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.