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\$
  • \$\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 '16 at 4:09
  • 1
    \$\begingroup\$ The cyclometric-complexity tag covers it. \$\endgroup\$ – 200_success Feb 10 '16 at 4:51
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\$
  • \$\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 '16 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 '16 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\$ – 200_success Feb 10 '16 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 '16 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\$ – 200_success Feb 10 '16 at 6:17

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.