Helpers are for small snippets of code

Manfred Stienstra

04 Jul 2007

Recently we contracted a local company to help out with a Rails project. Their primary job was to implement payments. During an audit or their work I found the following helper:

module CoursesHelper
  include ActiveMerchant::Billing 
  def issuers
    @gateway = IdealGateway.new(
         :merchant            => IDEAL_MERCHANT_ACCOUNT,
         :sub_id              => IDEAL_SUB_ID,
         :password            => IDEAL_PRIVATE_KEY_PASS,
         :private_cert        => IDEAL_PRIVATE_CERT,
         :language            => IDEAL_LANGUAGE,
         :private_key         => IDEAL_PRIVATE_KEY,
         :authentication_type => IDEAL_AUTHENTICATION_TYPE
    )
    
    list = Array.new()
    begin
      response = @gateway.issuers
      if response.success?
        list=Array.new()
        list.push({"issuerName"=>"Kies uw bank...", "issuerID"=>0})
        list=list + (response.params["list"])
      end
    rescue
      list = [{:issuerName=>"Kies uw bank...", :issuerID=>""},
      {:issuerName=>"ofline Simulator", :issuerID=>"00"}]
    end
    return list.map {|i| [ i["issuerName"], i["issuerID"] ] }

  end
  
end

Helpers are for keeping simple code out of your view. They should never include complicated logic. A helper should be so simple that you’d almost dare to deploy without testing.

Also, because it’s a good idea to keep coupling between classes to a minimum, the payment gateway should only be called from classes directly related to payment processing.

Finally, you don’t want such a large number of constants in your code. There are better ways to store the gateway configuration.

The contracter was asked to solve these issues. The gateway initialization was refactored to only appear once and the gateway object was assigned to a constant named IDEAL_GATEWAY. The code to find all issuers was moved to the Payment model.

class Payment < ActiveRecord::Base
  def self.ideal_issuers
    response = IDEAL_GATEWAY.issuers
    response.params["list"]
  end
end

module CoursesHelper
  
  def issuers
    list=Array.new()
    # 'Kies uw bank' means 'Choose your bank'
    list.push({"issuerName"=>"Kies uw bank...", "issuerID"=>0}) 
    list=list + Payment.ideal_issuers
    return list.map {|i| [i["issuerName"], i["issuerID"]] }
  end
  
end

Unfortunately, the issuers helper still looks more like Python than Ruby. I would have written it like this:

module CoursesHelper
  def issuers
    [['Kies uw bank...', 0]] + Payment.ideal_issuers.map \
      { |i|[i['issuerName'], i['issuerID']] }
  end  
end

I don’t think you should include instructions on how to use a drop-down as one of the options inside it. Furthermore, a default value makes no sense in this case. People always have to choose a bank, otherwise they can’t continue with the payment process.

After some refactoring I was left with the code below. I’ve also moved it to the helpers module for the payments controller, because that’s where the form is rendered.

module PaymentsHelper
  def issuers
    Payment.ideal_issuers.map { |i| [i['issuerName'], i['issuerID']] }
  end
end