0
\$\begingroup\$

Below is some code that I've been working to simplify, however I feel there is more that can be done. Please note that I did clean up the articles.search method.

class HomeController < ApplicationController
    @@articles_per_page = 6
     def index
        @paged_articles = articles.search(:user_id=> @@user_id, :page=>'1', :per_page=>@@articles_per_page)
        @tags = get_tags
        @next_page_url = url_for(:controller =>:home, :action => :page, :page_id=>  2)
        @page_title = "Recent articles"
     end

    def tags
      @paged_articles = articles.search(:page=>'1',:tags=>params[:tag_name], :per_page=>@@articles_per_page)
       @tags = get_tags
       @next_page_url = url_for(:controller =>:home, :action => :page, :tag_name => params[:tag_name], :page_id=>  2)
       @page_title = "Search articles by tags"
       render :index
     end

    def about
       @page_title = "About"
    end    

   private
    def get_tags
         articles.getTags 'param1', 'param2'
    end
   end

I simplified it this way

class HomeController < ApplicationController
  @@articles_per_page = 6
  @@first_page_articles_options = {:page=>'1', :per_page=>@@articles_per_page}
  @@second_page_url_options = {:controller =>:home, :action => :page, :page_id=>  2}


  def index
    @paged_articles = articles.search(@@first_page_articles_options)
    @tags = get_tags
    @next_page_url = url_for(@@second_page_url_options)
    @page_title = "Recent articles"
  end

  def tags
    @@first_page_articles_options[:tags]  = params[:tag_name]
    @paged_articles = articles.search(@@first_page_articles_options)
    @tags = get_tags

    @@second_page_url_options[:tag_name]  = params[:tag_name]
    @next_page_url = url_for(@@second_page_url_options)
    @page_title = "Search articles by tags"
    render :index
  end
   def about
      @page_title = "About"
  end

  private
  def get_tags
         articles.getTags 'param1', 'param2'
  end
end

So what do you think? Is there any way to make it more better and cleaner?

\$\endgroup\$
1
\$\begingroup\$

You can move urls and titles to helpers and articles search to separate method

class HomeController < ApplicationController
  # this methods can be used in view template
  helper_method :next_page_url, :page_title

  @@articles_per_page = 6
  @@first_page_articles_options = {:page=>'1', :per_page=>@@articles_per_page}
  @@second_page_url_options = {:controller =>:home, :action => :page, :page_id=>  2}


  def index
    @paged_articles = articles.search(@@first_page_articles_options)
    @tags = get_tags
  end

  def tags
    @paged_articles = load_articles
    @tags = get_tags

    render :index
  end
   def about
      @page_title = "About"
  end

  private

    def load_articles
      ...
    end 

  ...

  # helpers

  def next_page_url
    if filter_by_tags?
      url_for(@@second_page_url_options)  
    else
      @@second_page_url_options[:tag_name] = params[:tag_name]
      url_for(@@second_page_url_options)
    end    
  end

  def page_title
    if filter_by_tags?
      "Recent articles" 
    else
      "Search articles by tags"
    end  
  end

  def filter_by_tags?
    params[:tag_name].present?
  end
end
\$\endgroup\$
  • \$\begingroup\$ How do I do it? \$\endgroup\$ – Alexandre Aug 2 '12 at 14:17
  • \$\begingroup\$ update my answer \$\endgroup\$ – andrykonchin Aug 3 '12 at 21:40
  • \$\begingroup\$ There is no implementation of helper method yet. \$\endgroup\$ – Alexandre Aug 4 '12 at 5:29
  • \$\begingroup\$ next_page_url and page_title declared as helper methods and available in views, see apidock.com/rails/ActionController/Helpers/ClassMethods/… \$\endgroup\$ – andrykonchin Aug 6 '12 at 8:19
  • \$\begingroup\$ You should use constants where you use class variables here. \$\endgroup\$ – Alexey Aug 8 '12 at 19:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.