Concern or Class?

Ruby on Rails
1

I have a method that is shared across multiple controllers and is used to handle pagination in some admin reports. I just threw it into application controller, but I realize now this probably isn’t the best place for it.

Should I use a concern with just this method and include the concern in the appropriate controllers?

Or should I create a pagination class that is initialized in each of the controllers and then call methods like @pagination.period?

def set_start_times_and_periods
    if params[:period] == 'month'
      @period = 1.month
      @period_text = 'Month'
      @start_time = params[:date] ? Time.parse(params[:date]).beginning_of_month : (Time.now.beginning_of_month + 1.month).beginning_of_day
      @pagination_time = 6.months
      @pagination_text = '6 Months'
    elsif params[:period] == 'week'
      @period = 1.week
      @period_text = 'Week'
      @start_time = params[:date] ? Time.parse(params[:date]).beginning_of_week : (Date.parse('Monday') + 1.week).beginning_of_day
      @pagination_time = 8.weeks
      @pagination_text = '8 Weeks'
    else
      @period = 1.day
      @period_text = 'Day'
      @start_time = params[:date] ? Time.parse(params[:date]).beginning_of_day : 1.day.from_now.beginning_of_day
      @pagination_time = 14.days
      @pagination_text = '14 Days'
    end
  end
2

In general, I’d go with a class over a Concern if at all possible. This falls under the category of inheritance vs composition. In this particular case, the benefit of using a class would be the ability to replace this conditional with a different class for each case. First stage of the refactoring could look something like this:

module Period
  class Month < Struct.new(:date)
    def period
      1.month
    end

    def period_text
      'Month'
    end

    def start_time
      date ? Time.parse(date).beginning_of_month : (Time.now.beginning_of_month + 1.month).beginning_of_day
    end

    def pagination_time
      6.months
    end

    def pagination_text
      '6 Months'
    end
  end

  class Week < Struct.new(:date)
    def period
      1.week
    end

    def period_text
      'Week'
    end

    def start_time
      date ? Time.parse(date).beginning_of_week : (Date.parse('Monday') + 1.week).beginning_of_day
    end

    def pagination_time
      8.weeks
    end

    def pagination_text
      '8 Weeks'
    end
  end

  class Day < Struct.new(:date)
    def period
      1.day
    end

    def period_text
      'Day'
    end

    def start_time
      date ? Time.parse(date).beginning_of_day : 1.day.from_now.beginning_of_day
    end

    def pagination_time
      14.days
    end

    def pagination_text
      '14 Days'
    end
  end
end

Then, perhaps you might have a module that would have the following:

module PaginationPeriod
  extend ActiveSupport::Concern

  included do
    helper_method :pagination_period
  end

  def pagination_period
    case params[:period]
    when 'month'
      Period::Month.new(params[:date])
    when 'week'
      Period::Week.new(params[:date])
    else
      Period::Day.new(params[:date])
    end
end

From there you could look at other ways to restructure your code. Perhaps you’re only using one type in most places, or there’s some other way you can remove that conditional. If so, you could remove the module entirely and rely completely on the polymorphism here to handle this logic.

3

Thanks Sean this was helpful. This approach made me realize I should be looking at using structs more.