Skip to main content
added 144 characters in body
Source Link
200_success
  • 144.2k
  • 22
  • 188
  • 473

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.

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

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.

Source Link
200_success
  • 144.2k
  • 22
  • 188
  • 473

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