Reviewed code snippets with Best practices

Hello Team,

Last week, I got the chance to review the latest code commits from one of our projects.
This was a helpful step for me to learn good things from it.

Please suggest me, if there are any improvements needed.

  • Replace instance variable with local variable in partial.
<%= render partial: "review_confirmation", locals: { address: @address } %>

or

<%= render "review_confirmation", address: @address %>

so that use local variable in partial instead of instance variable.

  • Best practice to add semicolon ( ; ) at the end of every statement in js file.

  • Best practice to write css (styles) only in css folder rather than inline css in view file.

  • Best practice to Use spaces around operators, and curly braces {, }.

  • Do not use begin blocks where possible.

    def send_email_confirmation_of_payment
    begin
    email_subject = _('subj|payment confirmation for payment # %{reference}')
    Mailer.delay.payment_confirmation(email_subject, @user, open_items: @open_items)
    flash.now[:info] = _('msg|thanks for payment, email arrives shortly')
    rescue Exception => e
    logger.error "EMAIL_EXCEPTION;#{eval(Const::App.common_log_part)};
    flash.now[:info] = _('msg|thanks for payment')
    end
    end

we can be improved as:

def send\_email\_confirmation\_of\_payment
   email\_subject = _('subj|payment confirmation for payment # %{reference}')
   Mailer.delay.payment_confirmation(email_subject, @user, open\_items: @open_items)
   flash.now[:info] = _('msg|thanks for payment, email arrives shortly')
 rescue => e
   logger.error "EMAIL\_EXCEPTION;#{eval(Const::App.common_log_part)};
   flash.now[:info] = _('msg|thanks for payment')
 end
  • Arrange mass-assignments and methods in alphabetical order.

    attr\_accessor :credit_limit, :pay_date, :pay_amount, :credit_rep, :credit_cards, :echecks
    
    attr\_accessor :credit_cards, :credit_limit, :credit_rep, :echecks, :pay_amount, :pay_date
    

Also Methods name should be in alphabetical order.

  • Should use new syntax of ruby Hash.

  • Prefer {...} over do...end for single-line blocks. Avoid using {...} for multi-line blocks.

  • Dont Repeat same code:

    def get_availability_mock
    @materials.each do |material|
    material.in_stock = Quantity.new(1000000)
    material.end_lead_time = Date.today + 100
    material.min_order_qty = Quantity.new(1000)
    material.delivery_unit = Quantity.new(1000)
    qty = material.quantity ? material.quantity.value : nil
    case qty
    when 11000
    check_result = AvailabilityCheck::CheckResult.new
    check_result.date = Date.today
    check_result.quantity = Quantity.new(0)
    material.check_results = [ check_result ]
    when 12000
    check_result = AvailabilityCheck::CheckResult.new
    check_result.date = Date.today
    check_result.quantity = Quantity.new(2000)
    material.check_results = [ check_result ]
    else
    check_result = AvailabilityCheck::CheckResult.new
    check_result.date = Date.today
    check_result.quantity = Quantity.new(100000)
    material.check_results = [ check_result ]
    end
    end
    end

we can improve as:

def get\_availability\_mock
    @materials.each do |material|
      material.in_stock = Quantity.new(1000000)
      material.end_lead_time = Date.today + 100
      material.min_order_qty = Quantity.new(1000)
      material.delivery_unit = Quantity.new(1000)
      qty = material.quantity ? material.quantity.value : nil
      check\_result = AvailabilityCheck::CheckResult.new
      check\_result.date = Date.today
      case qty
      when 11000
        check\_result.quantity = Quantity.new(0)
      when 12000
        check\_result.quantity = Quantity.new(2000)
      else
        check\_result.quantity = Quantity.new(100000)
      end
      material.check_results = [ check\_result ]
    end
  end
  • There are two methods which have same code with different names.
 def download
   @open_items = @partner_cache.account_data[@selected_payer_number][@selected_company_code]
    analyze_assigned_payers
    filename = 'Open_items_list'
    headings, columns = open_items_list_headings_and_columns
    download_list(filename, @open_items.open_items, headings, columns)
  end

  def download_pay_invoices_list
    @open_items = @partner_cache.account_data[@selected_payer_number][@selected_company_code]
    analyze_assigned_payers
    filename = 'Open_items_list'
    headings, columns = open_items_list_headings_and_columns
    download_list(filename, @open_items.open_items, headings, columns)
  end

So we can improve the above by using ‘DRY’ (Dont Repeat Yourself) Concepts.

def common\_download\_list
  @open_items = @partner_cache.account_data[@selected_payer_number][@selected_company_code]
  analyze\_assigned\_payers
  filename = 'Open_items_list'
  headings, columns = open\_items\_list\_headings\_and\_columns
  download\_list(filename, @open_items.open_items, headings, columns)
end

def download
  common\_download\_list
end

def download\_pay\_invoices\_list
  common\_download\_list
end