1
\$\begingroup\$

The idea is that I am getting the params from a GET request in the form of a Hash (symbol→str) and I want to transform them into dates and booleans. For that reason, I created two private methods to_date and to_b.

class Report
  include ActiveModel::Model

  attr_accessor :start_date, :end_date, :show_sales, :show_daily_active_users,
                :show_registered_cc, :show_all_time_registered_cc, :show_users

  def initialize(params)
    params ||= {}
    @start_date = to_date(params, :start_date, Time.zone.today - 7.days)
    @end_date = to_date params, :end_date, Time.zone.today
    @show_sales = to_b params, :show_sales, true
    @show_daily_active_users = to_b params, :show_daily_active_users, true
    @show_registered_cc = to_b params, :show_registered_cc
    @show_all_time_registered_cc = to_b params, :show_all_time_registered_cc
    @show_users = to_b params, :show_users
  end

  def sales
     (...)
  end

  (...)

  private

  def to_date(params, key, default_val)
    if params.key?(key)
      Date.parse(params[key])
    else
      default_val
    end
  end

  def to_b(params, key, default_true = false)
    default_val = '1' if default_true
    params.fetch(key, default_val) == '1'
  end
end

The problem is that Rubocop says that the Assignment Branch Condition size for the method initialize is too high (15.81 over a limit of 15).

Is there anything I could change to fix that? I don't think that creating a new class just to do that is worth it.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the edit @Jamal. I was thinking that it could be a good idea to add a metrics and/or abc_metrics tags to this question, but I don't have enough Internet Points \$\endgroup\$
    – Daniel
    Feb 10, 2016 at 4:09
  • 1
    \$\begingroup\$ The cyclometric-complexity tag covers it. \$\endgroup\$ Feb 10, 2016 at 4:51

1 Answer 1

1
\$\begingroup\$

You could reduce branching and improve readability by requiring the default for to_b to be explicitly stated. (It seems a bit excessive to have a default for the default, don't you think?)

The branching is not my primary concern, though. The constructor seems to have a very copy-and-paste pattern. Metaprogramming could help here, I think. It would be nice to write…

class Report
  include ActiveModel::Model
  include ParamAccessors

  date_param_accessor(:start_date) { |today| today - 7.days }
  date_param_accessor :end_date
  bool_param_accessor :show_sales, true
  bool_param_accessor :show_daily_active_users, false
  bool_param_accessor :show_registered_cc, false
  bool_param_accessor :show_all_time_registered_cc, false
  bool_param_accessor :show_users, false

  def initialize(params)
    @params = params
  end

  def sales
     …
  end
end

This mixin could make that possible:

module ParamAccessors
  def self.included(base)
    base.extend ClassMethods
  end

  module ClassMethods
    def date_param_accessor(name, default=nil)
      attr_writer(name)
      define_method(name) do
        if instance_variable_defined?("@#{name}")
          instance_variable_get("@#{name}")
        else
          @params.fetch(name, default) or
          (yield Time.zone.today if block_given?) or
          return Time.zone.today
        end
      end
    end

    def bool_param_accessor(name, default)
      attr_writer(name)
      define_method(name) do
        if instance_variable_defined?("@#{name}")
          instance_variable_get("@#{name}")
        else
          @params.fetch(name, default)
        end
      end
    end
  end
end

This assumes that the object is transient, such that the constructor and the start_date/end_date methods will be called on the same day.

\$\endgroup\$
5
  • \$\begingroup\$ This refactoring looks great! I learned many things. Thanks! Out of curiosity, would you put the ParamAccessors module in the Concerns directory? \$\endgroup\$
    – Daniel
    Feb 10, 2016 at 5:39
  • \$\begingroup\$ Also, yes, the model is transient. Its function is just to provide data of several reports to a controller. \$\endgroup\$
    – Daniel
    Feb 10, 2016 at 5:51
  • \$\begingroup\$ That seems reasonable. In fact, if you have the module extend ActiveSupport::Concern, then you can write it slightly more gracefully, as in the docs. \$\endgroup\$ Feb 10, 2016 at 5:52
  • \$\begingroup\$ Sorry, there's something I don't understand. What is the point of the if instance_variable_defined?("@#{name}"); instance_variable_get("@#{name}")? With your solution, the attributes become methods instead of instance variables. \$\endgroup\$
    – Daniel
    Feb 10, 2016 at 6:07
  • 1
    \$\begingroup\$ Your original code had attr_accessor rather than attr_reader, so I assumed that you wanted those attributes to be writable, which means that when defining the attribute reader, you need to check the instance variable. \$\endgroup\$ Feb 10, 2016 at 6:17

This site is temporarily in read-only mode and not accepting new answers.

Not the answer you're looking for? Browse other questions tagged .