Условие рефакторинга с диапазоном значений

Question

Как можно реорганизовать этот бит кода Ruby on Rails?

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 

Answer 1

Если номера плана находятся в базе данных, и есть Plan модель, то вы могли бы просто сказать что-то вроде:

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

сейчас у вас есть доступ к полной информации о плане для следующего представления, чтобы вы могли показать им имя, описание, цену и ..., а существование плана хранится только в одном месте (т.е. в базе данных). Если вам не нужно @plan, то вы могли бы сказать:

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

Важным моментом является то, что должно быть ровно одна вещь, которая знает, что действительные планы и в любое время вы должны знать о планах, вы спросите, что одно и только одно.

мнение, что в конечном итоге вызова select_plan также использовать базу данных (вместо буквенных чисел от одного до восьми), чтобы получить действительные планы:

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

Добавить новый план в базу данных и все еще работает , Удалите/отключите план и все еще работает. Ваше программное обеспечение будет легче поддерживать, иметь меньше ошибок, быть более понятным, и вы приобретете новую хорошую привычку, а не плохую.

Answer 2

Я бы сделал что-то вроде этого.

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

Или как му слишком коротка предложил: сделать Plan реальная вещь. Это может быть модель базы данных или просто маленький рубин Класс:

# 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