Refactoring condition with a range of values

0
votes

How might one refactor this bit of Ruby on Rails code?

    def select_plan
        unless params[:plan] && (params[:plan] == '1' || params[:plan] == '2' || params[:plan] == '3' || params[:plan] == '4' || params[:plan] == '5' || params[:plan] == '6' || params[:plan] == '7' || params[:plan] == '8')
            flash[:notice] = "Please select a membership plan to register."
            redirect_to root_url
        end
    end
2
ruby-on-railsrubyif-statementswitch-statementrefactoring
Where do the valid plan numbers come from? Do they come from a database? Is there a constant that defines them somewhere? Are they magic numbers sprinkled throughout the code? - mu is too short
I ask because having the "6 is a valid plan" fact sitting only in a controller method or in several different places is what you need to refactor, not the cumbersome implementation you have in select_plan. Fix the underlying problem and select_plan will clean itself up as a side effect. - mu is too short
@muistooshort - Please excuse my level of experience. The valid plan numbers are present in the database. I am interested in what you have to say. I am not entirely sure of that you mean though. - Nathaniel Rand

2 Answers

3
votes

I would do something like this.

def select_plan
  unless params[:plan].in?('1'..'8')
    flash[:notice] = "Please select a membership plan to register."
    redirect_to root_url
  end
end

Or like mu is too short suggested: Make Plan a real thing. It might be a database model or just a small Ruby Class:

# in app/models/plan.rb
require 'set'
class Plan
  VALID_PLANS = Set.new('1'..'8').freeze

  def self.valid_plan?(plan)
    VALID_PLANS.include?(plan)
  end
end

# used like
def select_plan
  unless Plan.valid_plan?(params[:plan])
    flash[:notice] = "Please select a membership plan to register."
    redirect_to root_url
  end
end
0
votes

If the plan numbers are in the database and there is a Plan model then you could simply say something like:

@plan = Plan.find_by(:id => params[:plan])
if(!@plan)
  flash[:notice] = "Please select a membership plan to register."
  redirect_to root_url
end

Now you have access to the full plan details for the next view so that you can show them the name, description, price, ... and the existence of a plan is stored in only one place (i.e. the database). If you don't need @plan then you could say:

if(!Plan.where(:id => params[:plan]).exists?)
  ...
end

The important point is that there should be exactly one thing that knows what the valid plans are and any time you need to know about plans, you ask that one thing and only that one thing.

The view that ends up calling select_plan would also use the database (instead of the literal numbers one through eight) to get the valid plans:

<% Plan.order(...).each do |p| %>
  whatever you need to display the plan as an option...
<% end %>

Add a new plan to the database and everything still works. Remove/disable a plan and everything still works. Your software will be easier to maintain, have fewer bugs, be easier to understand, and you'll have gained a new good habit rather than a bad one.