4
\$\begingroup\$

I wrote the following Concern module. Is it good practice to perform all the calculations in the included scope? How can it be improved?

module Pagination
  module Helper
    extend ActiveSupport::Concern

    included do
      class << self
        def paged(params, custom_per_page = nil, custom_page = nil)
          per_page = params[:per_page] =
            custom_per_page || normalize_per_page(params[:per_page].to_i)
          page = normalize_page(self.all.size,
            custom_page || params[:page].to_i, per_page)

          self.paginate(page: page, per_page: per_page)
        end

        def default_per_page
          Reader::Application.config.custom.default_per_page_pagination
        end

        protected

        def normalize_per_page per_page
          if per_page < 1 || per_page > default_per_page
            return default_per_page
          end
          per_page
        end

        def normalize_page(size, page, per_page)
          if ((page - 1) * per_page) >= size
            page = (size / per_page.to_f).ceil
          end
          page > 0 ? page : 1
        end
      end
    end
  end
end

ActiveRecord::Base.send(:include, Pagination::Helper)
\$\endgroup\$
1
\$\begingroup\$

Since you're mixing methods into ActiveRecord::Base, you might as well extend it proper:

class ActiveRecord::Base
  def self.paged(...)
    ...
  end
end

No concerns necessary; you're just extending a class. Put it into a file in config/initializers and it'll load when the app spins up.

About what you're mixing in:

  • default_per_page doesn't make sense as a mixin. It's not the responsibility of every ActiveRecord-derived class to have a shortcut for fetching a global config value. The methods you add to a class via mixins should make as much sense, as if you'd added them directly to just that class.
    I'd place shortcuts like that in e.g. a separate helper module, and call it there.

  • Use Rails.configuration instead of <YourAppName>.config if you're aiming for reusable code.

  • Consider using hashes for method parameters. It makes calling your methods simpler and more descriptive.

  • Be consistent with your parentheses. You declaration of normalize_per_page omits the parentheses, but your declaration of normalize_page includes them. Personally, I always use parens in method declarations.

  • Do not access params in a model. Keep that in the controller, and only the controller; it's its job. This also means keeping the normalize_page method (which I'd call constrain_page_number, because it doesn't normalize a page, but constrains a number) in the controller. You can simply add it to ApplicationController to have available:

    # in app/application_controller.rb
    
    protected
    
    def constrain_page_number(number, record_count, per_page = nil)
      per_page ||= PagniationHelpers.default_per_page # our helper module
      [1, number, (record_count / per_page).floor + 1].sort[1]
    end
    

    However, in my view, your controller should send a redirect if the page number in out of bounds - don't just rewrite it behind the scenes. So you might want something like this instead:

    def valid_page_number?(number, record_count, per_page = nil)
      per_page ||= PagniationHelpers.default_per_page # our helper module
      max = (record_count / per_page).floor + 1
      (1..max).cover?(number)
    end
    
  • Now, something that'd might make more sense to include in every model would be a max_page method. It'd allow you to more easily check the page number in the controller.
    Of course, such a method would (unlike the above) have to rely on just the default scope when counting the number of records. Still, it might be useful.

In all I end with something like this (plus the controller method above):

module PagniationHelpers
  module_function

  def default_per_page
    Rails.config.custom.default_per_page_pagination
  end
end

class ActiveRecord::Base
  class << self
    def paged(page, per_page = nil)
      per_page ||= PagniationHelpers.default_per_page
      self.paginate(page: page, per_page: per_page)
    end
  end
end

Buuuut of course, it looks like you're using the will_paginate gem. In that case, forget the code above. Instead, do this in an initializer file somewhere:

WillPaginate.per_page = 10 # or whatever default you want

and in ApplicationController do:

def max_page_number(record_count, per_page = nil)
  per_page ||= WillPaginate.per_page
  (record_count / per_page).floor + 1
end

def valid_page_number?(number, record_count, per_page = nil)
  max = max_page_number(record_count, per_page)
  (1..max).cover?(number)
end

def constrain_page_number(number, record_count, per_page = nil)
  max = max_page_number(record_count, per_page)
  [1, number, max].sort[1]
end

And in your controller actions you can do something like

def index
  page = params[:page].try(:to_i) || 1
  unless valid_page_number?(page, Post.count)
    redirect_to posts_path(page: constrain_page_number(page, Post.count))
  end
  @posts = Post.paginate(:page => page)
  #...
end
| improve this answer | |
\$\endgroup\$

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.