Refactoring-Bedingung mit einem Bereich von Werten

Question

Wie könnte man dieses Bit von Ruby on Rails-Code umgestalten?

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

Wenn die Plannummern in der Datenbank vorhanden sind, und es ist ein Plan Modell dann könnte man einfach sagen, so etwas wie:

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

Jetzt Sie haben Zugriff auf die vollständigen Plandetails für die nächste Ansicht, so dass Sie ihnen den Namen, die Beschreibung, den Preis usw. anzeigen können. Das Vorhandensein eines Plans wird nur an einem Ort (dh in der Datenbank) gespeichert. Wenn Sie nicht @plan brauchen dann könnte man sagen:

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

Der wichtige Punkt ist, dass es genau eine Sache sein, die weiß, was die gültigen Pläne sind und jederzeit Sie brauchen, um über Pläne zu wissen, fragen Sie, dass eine Sache und nur diese eine Sache.

Die Ansicht, dass select_plan auch die Datenbank (anstelle den wörtlichen Zahlen eins bis acht) verwenden würde Aufruf endet, um die gültigen Pläne zu erhalten:

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

einen neuen Plan zur Datenbank hinzufügen und alles noch funktioniert . Entferne/deaktiviere einen Plan und alles funktioniert noch. Ihre Software wird einfacher zu warten sein, weniger Bugs haben, leichter zu verstehen sein, und Sie werden eine neue gute Angewohnheit und keine schlechte bekommen.

Answer 2

Ich würde so etwas tun.

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

Oder wie mu ist zu kurz vorgeschlagen: Machen Sie Plan eine reale Sache. Es könnte ein Datenbankmodell oder nur eine kleine Ruby-Klasse sein:

# 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