Ruby: Refactoring code #1

In these series I'll write some posts on refactoring ruby code.

No matter what language you write, there are always quite similar approach to refactoring.

Refactoring means changing code, improving it's readability and scalability, without functional modification.


Here are some basic steps:
0. Find the code that smells.

1. Check if code has enough test coverage. Write tests if no coverage.
2. Refactor code. Do it by small pieces. Don't try to refactor everything at once. Make this process iterative.
3. Run tests.
4. Refactor tests.
5. Commit changes.


It's really important to refactor code step by step, don't try to refactor big pieces of code. Otherwise you'll lose the clue.

The other important thing is not to change functionality. Even if you see the code has a bug, fix it right after you are done with refactoring.

Testing is very important during whole process of refactoring, you can't be sure for your code without tests.


So, lets jump right to the examples.

First, we need to find stinky code. Most of that you can find in your controllers.

Here is the first candidate for refactoring, we have a CategoriesController, with show method:


  def show
    @category = Category.find(params[:category_id], :include=>[:category_stats])
    @region = Region.find_by_id(params[:region_id]) unless params[:region_id].blank?
    @action = Action.find_by_id(params[:action_id]) unless params[:action_id].blank?

    @alternatives = @category.alternative_categories.counter_caches.to_a

    CategoryStat.category_opened(@category, request.remote_ip)
    key = {:category_id=>@category.id}
    key.merge!(:region_id=>@region.id) if @region
    set_scope_id(key)

    unless show_subcategories?(@category, @region)
      common_args = {:page => params[:page], :per_page => ADS_PER_PAGE,
        :include => [:custom_attributes, :price_unit, :category, :assets, :region],
        :order => order, :conditions=>filter_conditions}

      ad_scope = AdList.category_or_children(@category)
      ad_scope = ad_scope.active
      ad_scope = ad_scope.action(@action.id) if @action
      ad_scope = ad_scope.region(@region) if @region
      ad_scope = ad_scope.created_after(DateFilter.value(date_filter)) if date_filter_applied?

      if region_selected?
        @regional_ads = ad_scope.region(current_region).paginate(common_args)
        ad_scope = ad_scope.not_in_ids(@regional_ads.collect{|a| a.id}) unless @regional_ads.empty?
      end

      @ads = ad_scope.paginate(common_args)
    end
  end



As we can see, the method is really big, and this is kind of problem. So the first step would be getting rid of show_subcategories? conditions by spliting it to two different methods.

So we have the following:



def messages
    @category = Category.find(params[:category_id], :include=>[:category_stats])
    @region = Region.find_by_id(params[:region_id]) unless params[:region_id].blank?
    @action = Action.find_by_id(params[:action_id]) unless params[:action_id].blank?

    @alternatives = @category.alternative_categories.counter_caches.to_a

    CategoryStat.category_opened(@category, request.remote_ip)
    key = {:category_id=>@category.id}
    key.merge!(:region_id=>@region.id) if @region
    set_scope_id(key)
    
    common_args = {:page => params[:page], :per_page => ADS_PER_PAGE,
        :include => [:custom_attributes, :price_unit, :category, :assets, :region],
        :order => order, :conditions=>filter_conditions}

      ad_scope = AdList.category_or_children(@category)
      ad_scope = ad_scope.active
      ad_scope = ad_scope.action(@action.id) if @action
      ad_scope = ad_scope.region(@region) if @region
      ad_scope = ad_scope.created_after(DateFilter.value(date_filter)) if date_filter_applied?

      if region_selected?
        @regional_ads = ad_scope.region(current_region).paginate(common_args)
        ad_scope = ad_scope.not_in_ids(@regional_ads.collect{|a| a.id}) unless @regional_ads.empty?
      end

      @ads = ad_scope.paginate(common_args)
  end

  def show
    @category = Category.find(params[:category_id], :include=>[:category_stats])
    @region = Region.find_by_id(params[:region_id]) unless params[:region_id].blank?
    @action = Action.find_by_id(params[:action_id]) unless params[:action_id].blank?

    @alternatives = @category.alternative_categories.counter_caches.to_a

    CategoryStat.category_opened(@category, request.remote_ip)
    key = {:category_id=>@category.id}
    key.merge!(:region_id=>@region.id) if @region
    set_scope_id(key)
  end



So now we have a small duplication, here is how we can handle this:




before_filter :find_category_region_action, :only => [:show, :messages]
  
  def find_category_region_action
    @category = Category.find(params[:category_id], :include=>[:category_stats])
    @region = Region.find_by_id(params[:region_id]) unless params[:region_id].blank?
    @action = Action.find_by_id(params[:action_id]) unless params[:action_id].blank?

    @alternatives = @category.alternative_categories.counter_caches.to_a

    CategoryStat.category_opened(@category, request.remote_ip)
    key = {:category_id=>@category.id}
    key.merge!(:region_id=>@region.id) if @region
    set_scope_id(key)
  end



So now we have two clean methods with no duplication.
Ярлыки: ,