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.

Wrap long text with ruby

Here is a code snipet on how to wrap long texts.

It actually do long lines ('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa') and ('adas asdas asdas dad asdasd ad') wrapping with ' ' and




Thanks to http://blog.macromates.com/2006/wrapping-text-with-regular-expressions/

Data migrations with rails

data_migration plugin

Sinatra. First aplication

Recently tried a sinatra framework for a small web app.

Sinatra is actually cool thing. It's easy to start and configure. But I still not sure if it fits for larger than small applications.

I've coded a small app with almost the same structure as a rails application. It could be useful to get started.

Testing Rails plugins

Here's a short list on how to test your plugins.

1) Structure

vendor/
  plugins/
    yourplugin/
      test/
        helper.rb
        your_test.rb
        database.yml
        schema.rb
      init.rb
      Rakefile

2) Rakefile


3) helper.rb


4) your_test.rb


5) schema.rb


6) Running tests
Go to your plugin root folder and run
$ rake test

To see full example, refer to http://github.com/balepc/ip_geolocation/

Here are some other useful links:

Rails plugin testing guide
Another guide

Geolocation by IP address

Detecting user's location by his ip address is quite a common task in many web applications. You can find lot's of them in Internet. Here are a short list:

http://hostip.info/
http://ipinfodb.com/
http://www.wipmania.com/


All of them have pretty simple interface. Here is a ruby wrapper for that http://github.com/balepc/ip_geolocation