Refactor code that includes "double pipe" and "instance variable" using metaprogramming

Question

I have two methods:

def ios_ids
  @ios_ids ||= Array(GcmToken.find_by(users_id: "#{@event.user_id}", os_type: 'ios', alive: true).try(:reg_id))       
end

def android_ids
  @android_ids ||= Array(GcmToken.find_by(users_id: "#{@event.user_id}", os_type: 'android', alive: true).try(:reg_id)) 
end

I want to refractor them in to something like below

%w(android ios).each do |os_type|
  define_method(:"#{os_type}_ids") { "@#{os_type}_ids" ||= Array(GcmToken.find_by(users_id: "#{@event.user_id}", os_type: os_type, alive: true).try(:reg_id))}
end

But It doesn't work

Anyone have any answers or better solution?

Answer 1

Using that kind of metaprogramming is really heavy-handed, especially when manipulating instance variables like that. Normally when you go down that path it's because you've got a lot of situations like that which need to be tidied up.

Let's tackle this in smaller steps:

def ios_ids
  @ios_ids ||= Array(GcmToken.find_by(users_id: "#{@event.user_id}", os_type: 'ios', alive: true).try(:reg_id))       
end

There's some really odd stuff happening here, like the "#{x}" anti-pattern, that's quoting a single value which is almost always pointless. In cases where you absolutely need a string, use .to_s on the value in question.

This also loads the model and tries to fetch an attribute from it. That's extremely wasteful. It also packages it up using the irregular Array(...) notation which is rarely used. [ ... ] is preferred.

So cleaning this up a little you get:

def ios_ids
  @ios_ids ||= GcmToken.where(
    users_id: @event.user_id,
    os_type: 'ios',
    alive: true
  ).pluck(:reg_id)
end

That boils it down a lot. Now it's just fetching all of the associated reg_id values from that GcmToken model. If you have a User has_many :gcm_tokens and Event belongs_to :user, which should be the case given the data here, then you can clean it up even more:

def ios_ids
  @ios_ids ||= @event.user.gcm_tokens.where(
    os_type: 'ios',
    alive: true
  ).pluck(:reg_id)
end

You can clean this up even more with a simple scope declaration:

scope :alive_for_os_type, -> (os_type) {
  where(os_type: os_type, alive: true)
}

Then it becomes even smaller:

def ios_ids
  @ios_ids ||= @event.user.gcm_tokens.alive_for_os_type('ios').pluck(:reg_id)
end

That's getting pretty small. At that point reducing this with define_method is over-kill, but if you really want to, then do this:

OS_TYPES = %w[ android ios ].freeze

OS_TYPES.each do |os_type|
  method_name = "#{os_type}_ids".to_sym
  instance_var = "@#{method_name}".to_sym

  define_method(method_name) do
    instance_variable_get(instance_var) or
      instance_variable_set(
        instance_var,
        @event.user.gcm_tokens.where(
          os_type: 'ios',
          alive: true
        ).pluck(:reg_id)
      )
  end
end

That ends up being significantly more mess and code than just boiling down the implementation of each to a more minimal form. If you had dozens of types maybe you'd want to do this, but honestly, what about this:

def platform_ids(os_type)
  @platform_ids ||= { }

  @platform_ids[os_type] ||= @event.user.gcm_tokens.alive_for_os_type(os_type).pluck(:reg_id)
end

One method that can handle N types, all you have to do is specify which one. Sometimes special-purpose methods aren't worth the fuss.

Answer 2

You want to use Object#instance_variable_set and Object#instance_variable_get:

%w(android ios).each do |os_type|
  define_method "#{os_type}_ids" do
    instance_variable_get("@#{os_type}_ids") ||
    instance_variable_set("@#{os_type}_ids", Array(GcmToken.find_by(users_id: "#{@event.user_id}", os_type: os_type, alive: true).try(:reg_id)))
  end
end