An application has controller action methods with repeating patterns.
Essentially, on a toggle items are added or removed, a counter amended and a toggle state changed. The methods are within a shared concern. note these may not necessarily be referring to objects of the same class
This is one such method:

def toggle_territory_criteria
  if params[:criteria] == "none" 
    if params[:toggle].to_i == 0
      @region_none_active = 0
      @regional_total = params[:regional_total].to_i - params[:value].to_i
    else
      @region_none_active = 1
      @regional_total = params[:regional_total].to_i   params[:value].to_i
    end
  else
    if params[:toggle].to_i == 0
      @active_regionminor_ids.delete(params[:criteria].to_i)
      @regional_total = params[:regional_total].to_i - params[:value].to_i
    else
      @active_regionminor_ids = params[:regionminors] << params[:criteria].to_i
      @regional_total = params[:regional_total].to_i   params[:value].to_i
    end
  end
end

This can be refactored and fully abstracted in the following three methods:

def toggle_state(attribute)
  attribute = false
  if params[:toggle].to_i == 0
    attribute = true
  end
end

def toggle_collection(id_collection, criteria_attribute)
  id_collection << criteria_attribute
  if params[:toggle].to_i == 0
    id_collection.delete(criteria_attribute)
  end
end

def toggle_sums(sum_value, original_sum, new_value)
  sum_value = original_sum   new_value
  if params[:toggle].to_i == 0
    sum_value = original_sum - new_value
  end
end

leading to a succinct concern method

def toggle_territory_criteria_too
  if params[:criteria] == "none"
    toggle_state(@region_none_active)
    toggle_sums(@regional_total, params[:regional_total].to_i, params[:value].to_i)
  else
    toggle_collection(@active_regionminor_ids, params[:criteria].to_i)
    toggle_sums(@regional_total, params[:regional_total].to_i, params[:value].to_i)
  end
end 

However, the resulting variables are not accessible in the originating method nor in the views (the concern method was using instance variables for this purpose).

What would be a proper way to execute this refactoring to service the controller action?

CodePudding user response：

In Ruby (and thus Rails, of course), a method never changes the values of the objects it receives as arguments, except when destructive methods are used.

In your case, when you run toggle_state(@region_none_active), the method toggle_state never changes @region_none_active, for example. What toggle_state does is only to return the value of the last executed line, namely either the value of if params[:toggle].to_i == 0 if it is false or the returned value of the line of attribute = true, which is true. attribute in the method toggle_state is a local variable and changing it has nothing to do with @region_none_active.

Similarly, the method toggle_sums never changes anything and simply returns either Numeric or false(!), depending on a set of the given value as the attributes. I am sure you would not want the value false from the method!

Instead, the following would set the instance variable (at the caller's side), for example, of @region_none_active in your concern method.

  @region_none_active = toggle_state(@region_none_active)

You need similar modifications for the other methods in your code. This specification is Ruby's fundamental principle, and so consult the Ruby reference or textbook for detail (but I am sure you know that?).

Anf finally, if you want to access the instance variable @region_none_active from your view, you probably would need to write your own getter method for it, like

  # Your model
  attr_reader :region_none_active
  # And access it from your model "mdl" like
  #   mdl.region_none_active

where attr_reader is a Ruby built-in alias of
def region_none_active; @region_none_active; end

Note I would recommend to rewrite your toggle_state as follows for the sake of clarity, though what it does is identical to your code (and to add, a better name for the method would be something like desired_state? according to Ruby's convention for method names):

def toggle_state(attribute)
  params[:toggle].to_i == 0
end

Another note is, the two methods id_collection << and id_collection.delete in your code are destructive and hence modify the caller's object. So you need to handle them with care. To be honest, I would not recommend to use such destructive methods in a method because to change the caller's object directly in a user method would usually make your code less organised and hard to read. I'd stick to non-destructive methods only in general.