Calculation in Concern module using Rails

Question

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)

Answer 1

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