Search code examples
ruby-on-railsruby-on-rails-4instance-variablesclass-method

Variable available to class methods (within Concerns)


I created a Rails Concern for my ActiveModel called List

When I run Product.all from the Rails Console, I get:

NameError: undefined local variable or method `parameters' for Product:Class

When parameters is changed to @parameters I get this error:

NoMethodError: undefined method `include?' for nil:NilClass

Possible Solutions

Which is better, using a constant PARAMETERS or @@Parameters? Pros and Cons?

Code

module List
  extend ActiveSupport::Concern

  require 'csv'

    parameters =  [ 
                  :visible,
                  :desc,
                  :value,
                  ]

    attr_accessor(*parameters)

  def initialize(attributes = {})
    attributes.each do |name, value|
      send("#{name}=", value)
    end
  end

  def persisted?
    false
  end

  module ClassMethods 
    def all   
      list = []
      filename = File.join(Rails.root,"app/models/data/#{self.name.downcase}.csv")
      CSV.foreach(filename, headers: true) do |row|
        list << self.new(row.select{|key,_| parameters.include? key.to_sym })
      end
      return list
    end

    def visible
      list = []
      filename = File.join(Rails.root,"app/models/data/#{self.name.downcase}.csv")
      CSV.foreach(filename, headers: true) do |row|
          list << self.new(row.select{|key,_| parameters.include? key.to_sym })  if row['visible']=='1'
      end
      return list
    end
  end
end

Solution

  • For a quick solution, to make Product.all and Product.visible work with the least amount of modification to your existing code, you can define a parameters method inside module ClassMethods. For example:

    def parameters
      @parameters ||= [:visible, :desc, :value]
    end
    

    This method solution can also serve as a long-term solution if you plan to use the parameters outside of the concern, or if a subclass might want to define its own parameters.

    However, if the parameters are only meant to be used inside this concern, and this data will never change, at least not through any application logic, then a constant would be the best solution because it conveys the proper meaning to the reader. I would also freeze it to prevent modification:

    PARAMETERS = [:visible, :desc, :value].freeze
    

    Another option, as Rich mentioned, is to define a class variable. Note that the constant will work whether you define it inside the List module, or inside the ClassMethods module. However, a class variable will only work inside the ClassMethods module if you want Product to be able to call it as parameters.

    Also, note that self is implied in any method within ClassMethods, so you don't need to specify it. If you defined a parameters method, it would be considered a Product class method, and if you used parameters within the all method, it would refer to the class method, not an instance method as suggested by Rich.

    Class variables are generally discouraged in Ruby because their side effects are often misunderstood. The Ruby Style Guide recommends avoiding them: https://github.com/bbatsov/ruby-style-guide#no-class-vars

    As for speed, I compared the method and constant solutions, and it looks like the constant is faster:

    require "benchmark/ips"
    
    PARAMETERS = [:visible, :desc, :value].freeze
    
    def parameters
      @parameters ||= [:visible, :desc, :value]
    end
    
    def uses_constant
      puts PARAMETERS
    end
    
    def uses_method
      puts parameters
    end
    
    Benchmark.ips do |x|
      x.report("constant") { uses_constant }
      x.report("method") { uses_method }
      x.compare!
    end
    

    The result:

    Comparison:
      constant:  45256.8 i/s
      method:    44799.6 i/s - 1.01x slower