Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

Ruby on Rails 3.2.3, Ruby 1.9

Here is the an action of a controller in Ruby on Rails. There are two conditions: if parameter :tag_name exists and if it doesn't. How do I simplify this?

def page
        page  = params[:page_id]

        if !params[:tag_name].nil?
          @paged_articles = articles.search(:page=>page, :tags=>params[:tag_name], :per_page=>@@articles_per_page)
        else
          @paged_articles = articles.search(:page=>page, :per_page=>@@articles_per_page)
        end

        if @paged_articles.size>0
            page.succ!
            if !params[:tag_name].nil?
              has_more_articles = articles.search(:page=>page, :tags=>params[:tag_name], :per_page=>@@articles_per_page)            
             else  
              has_more_articles = articles.search( :page=>page, :per_page=>@@articles_per_page)
            end


            if has_more_articles.size>0
              if !params[:tag_name].nil?
                @next_page_url = url_for(:controller =>:home, :action => :page, :tags=>params[:tag_name], :page_id=>  page)
              else
                @next_page_url = url_for(:controller =>:home, :action => :page, :page_id=>  page)
              end
            else
              @next_page_url = nil
            end

          respond_to() do |format|
            format.js
          end

        end
      end
share|improve this question
what does page.succ! do? – Speransky Danil Aug 2 '12 at 8:17
it increments the value of page. – Alexandre Aug 2 '12 at 8:21
But page is an integer (id), or I am wrong? – Speransky Danil Aug 2 '12 at 8:25
yes. However it works. – Alexandre Aug 2 '12 at 8:26
1  
the first and always useful advice: move absolutely all the code you can to the models and keep the controller simple. – tokland Aug 2 '12 at 8:48
show 4 more comments

migrated from stackoverflow.com Aug 2 '12 at 19:07

1 Answer

up vote 1 down vote accepted 
def page
  page  = params[:page_id]

  options = { :per_page => @@articles_per_page, :page => page }
  opitons[:tags] = params[:tag_name] if params[:tag_name]

  @paged_articles = articles.search(options)

  if @paged_articles.any?   
      options[:page] = page.succ!
      has_more_articles = articles.search(options)

      if has_more_articles.any?
        options = { :controller => :home, :action => :page, :page_id =>  page }
        opitons[:tags] = params[:tag_name] if params[:tag_name]
        @next_page_url = url_for(options)
      else
        @next_page_url = nil
      end

    respond_to do |format|
      format.js
    end
  end
end
share|improve this answer
1  
opitons.merge! (:tags => params[:tag_name]) if !params[:tag_name].nil? => options[:tags] = params[:tag_name] if params[:tag_name] – DNNX Aug 2 '12 at 8:33
respond_to() => respond_to – DNNX Aug 2 '12 at 8:33
I just was working with author's code... :) – Speransky Danil Aug 2 '12 at 8:34
Fixed. Thank you :) – Speransky Danil Aug 2 '12 at 8:35
2  
@paged_articles.size > 0 and has_more_articles.size > 0 can be written as @paged_articles.any? and has_more_articles.any?. – Jakob S Aug 2 '12 at 8:44
show 12 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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