CODEWITHSUNDEEP
ruby-on-railsrubyrefactoring
ardavis
ardavis

Reputation: 9895

How can I refactor these common controller methods?

I have a few controller methods that are extremely similar and I was wondering what the best way to refactor them would be. First thing that comes to mind would be somehow passing in two blocks to a helper method, but I'm not sure how to do that either.

def action_a
  if @last_updated.nil?
    @variable_a = @stuff_a
  else
    @variable_a = (@stuff_a.select{ |item| item.updated_at > @last_updated }
  end
end

def action_b
  if @last_updated.nil?
    @variable_b = @stuff_b.some_method
  else
    @variable_b = @stuff_b.some_method.select{ |stuff| item.updated_at > @last_updated }
  end
end

It just seems like I'm constantly checking if @last_updated is nil (I set the @last_updated instance variable in a before_filter. If I could somehow pass the stuff inside the if as a block and the stuff in the else as another block, then I could remove the if @last_updated.nil? duplication?

What is the best way of accomplishing this for many methods?

Update

Where I specify @stuff_a and @stuff_b, they are always returning an array (since I use .select).

Upvotes: 0

Views: 230

Answers (3)

sawa
sawa

Reputation: 168101

You need to extract your condition in a separate def block and use it later on:

def select_updates a
  @last_updated.nil? ? a : a.select{ |item| item.updated_at > @last_updated }
end
def action_a; @variable_a = select_updates(@stuff_a) end
def action_b; @variable_b = select_updates(@stuff_b.some_method) end

Upvotes: 1

sameera207
sameera207

Reputation: 16629

AS I can see, you could do the followings

have two scope for each

Ex: 

class Stuff < ActiveRecord::Base
  scope :updated_at, lambda {|updated_date|
    {:conditions => "updated_at > #{updated_date}"}
  }
end


class Item < ActiveRecord::Base
  scope :updated_at, lambda {|updated_date|
    {:conditions => "updated_at > #{updated_date}"}
  }
end

in your controller do this

def action_a
  @variable_a = update_method(@stuff_a)
end

def action_b
  @variable_b = update_method(@stuff_b)
end

private
def update_method(obj)
  result = nil
  if @last_updated.nil?
    result = obj.some_method 
  else    
    result = obj.some_method.updated_at(@last_updated) 
  end  
  result 
end

HTH

Upvotes: 0

Sergio Tulentsev
Sergio Tulentsev

Reputation: 230336

Take a look at this. It's DRYer and should yield identical results.

def action_a
  do_the_processing :"@variable_a", @stuff_a
end

def action_b
  do_the_processing :"@variable_b", @stuff_b.some_method
end

private
def do_the_processing var_name, collection
  if @last_updated.nil?
    instance_variable_set var_name, collection
  else
    instance_variable_set var_name, collection.select{ |item| item.updated_at > @last_updated }
  end
end

Update

And here's the two blocks approach (just for fun) (uses 1.9's stabby lambda syntax)

def action_a
  check_last_updated is_nil: -> { @variable_a = @stuff_a },
                     is_not_nil: -> { @variable_a = (@stuff_a.select{ |item| item.updated_at > @last_updated } }
end

def action_b
  check_last_updated is_nil: -> { @variable_b = @stuff_b.some_method },
                     is_not_nil: -> { @variable_b = @stuff_b.some_method.select{ |stuff| item.updated_at > @last_updated } }
end

private
def check_last_updated blocks = {}
  if @last_updated.nil?
    blocks[:is_nil].try(:call)
  else
    blocks[:is_not_nil].try(:call)
  end
end

Upvotes: 3

Related Questions