开发者

Refactoring of many if statements for params[...], inside controller action

I have such code, for making chain selects in my form View for index action:

<%= form_tag do %>
    <%= collection_select(*@brands_select_params) %>
    <%= collection_select(*@car_models_select_params) %>
    <%= collection_select(*@production_years_select_params) %>
    <% # Пока еще никто ничего не выбрал %>
<%= submit_tag "Send", :id => "submit", :name => "submit" %>

And my controller:

class SearchController < ApplicationController
  def index
    @brands = Brand.all
    @car_models = CarModel.all

    if (params[:brand].blank?)
      @brands_select_params = [:brand, :id, @brands, :id, :name, :prompt => "Выбирай брэнд"]

      if params[:car_model].blank?
        @car_models_select_params = [:car_model, :id, @car_models, :id, :name, { :prompt => "Model" }, \
                                   { :disabled => "disabled" }]
        @production_years_select_params = [:production_year, :id, @car_models, :id, :name, { :prompt => "Year" }, \
                                         { :disabled =&开发者_如何学JAVAgt; "disabled" }]
      end
    else
      @brands_select_params = [:brand, :id, @brands, :id, :name, { :selected => params[:brand][:id] } ]
      if params[:car_model].blank?
        @car_models_select_params = [:car_model, :id, Brand.find(params[:brand][:id]).car_models, :id, :name, \
                                   { :prompt => "And model now" } ]
        @production_years_select_params = [:production_year, :id, @car_models, :id, :name, { :prompt => "Year" }, \
                                         { :disabled => "disabled" } ]
      else
        @car_models_select_params = [:car_model, :id, Brand.find(params[:brand][:id]).car_models, :id, :name, \
                                   { :selected => params[:car_model][:id] } ] unless params[:car_model][:id].empty?
        @production_years_select_params = [:production_year, :id, CarModel.find(params[:car_model][:id]).production_years, :id, :year, \
                                         { :prompt => "And year now" } ] unless params[:car_model][:id].empty?
      end
    end
  end
end

As you can see, too many ifs in my controller code. And i gonna add more conditions there. After that anyone who read that code will get brain corruption. So i just wanna make it in real Ruby way, but don't know how. Please, help, guys. How should i refactor this bunch of crap?


I think a big part of the problem is you're doing too much in your controller. Generating markup (and IMO that includes building parameter lists for form helpers) should be done in views and view helpers. So:

module SearchHelper
  def brand_select brands, options={}
    collection_select :brand, :id, brands, :id, :name, :options
  end

  def car_model_select car_models, options={}
    collection_select :car_model, :id, car_models, :id, :name, options
  end

  def production_year_select years, options={}
    collection_select :production_year, :id, years, :id, :year, options
  end
end

Then you can cut your controller down to this:

def index
  @brands     = Brand.all
  @car_models = CarModel.all

  @selected_brand_id      = params[:brand]      && params[:brand][:id]
  @selected_car_model_id  = params[:car_model]  && params[:car_model][:id]

  @production_years = @selected_car_model_id ?
    [] : CarModel.find(@selected_car_model_id).production_years
end

And in your view:

<%= brand_select @brands, :prompt   => "Выбирай брэнд",
                          :selected => @selected_brand_id
%>
<%= car_model_select @car_models, :prompt   => "Model",
                                  :selected => @selected_car_model_id
%>
<%= production_year_select @production_years, :prompt   => "Year",
                                              :selected => @selected_car_id
%>

I suspect you could simplify this even more using form_for and fields_for and get rid of the helpers entirely, but it depends a bit on how your model associations are set up.


There is no obvious solution to this kind of problem.

Generally, I try to keep the if / else architecture very clear and export all code into separate methods. 2 advantages here:

  • readability

  • easier unit testing

For your case, it would be:

class SearchController < ApplicationController
  def index
    @brands = Brand.all
    @car_models = CarModel.all

    if (params[:brand].blank?)
      @brands_select_params = [:brand, :id, @brands, :id, :name, :prompt => "Выбирай брэнд"]
      if params[:car_model].blank?
        @car_models_select_params, @production_years_select_params =  get_card_model(@car_models)
      end
    else
      @brands_select_params = [:brand, :id, @brands, :id, :name, { :selected => params[:brand][:id] } ]
      if params[:car_model].blank?
        @car_models_select_params, @production_years_select_params = foo_method(@car_models)
      else
        @car_models_select_params, @production_years_select_params = bar_method
      end
    end
  end

  def get_card_model car_models
   [
    [:car_model, :id, car_models, :id, :name, { :prompt => "Model" }, { :disabled => "disabled" }],
    [:production_year, :id, car_models, :id, :name, { :prompt => "Year" }, { :disabled => "disabled" }]
    ]
  end
end
0

上一篇:

下一篇:

精彩评论

暂无评论...
验证码 换一张
取 消

最新问答

问答排行榜