Ruby string literals VS Value objects. Overengineering?


Im my previous article I showed how you can use Rails 5 Attributes API with JSONB and value objects to improve the design of your application.

Today I want to show you how Attributes API can be applied to refactor Primitive Obsession anti pattern.

Primitive data types are the basic built-in building blocks of a language. They're usually typed as int, string, or constants. As creating such fields is much easier than making a whole new class, this leads to abuse. Therefore, this makes this antipattern one of the most common ones.

  • Using primitive data types to represent domain ideas. For example, using an integer to represent an amount of money or a string for a phone number.

  • Using variables or constants for coding information. An oft-encountered case is using constants for referring to users roles or credentials (like USER_ADMIN = 1;).

  • Using strings as field names in data arrays.

Lets take a look at this code.


class Plan::Channel < ApplicationRecord

  CALCULATION_METHODS = %w(

    sliding_scale_net_origination_fee

    sliding_scale_origination_volume

    flat

    spread

  ).freeze



  TYPES = %w(

    direct

    broker

    correspondent

    bdr

    non_funded_origination

  ).freeze



  self.inheritance_column = nil



  belongs_to :plan



  TYPES.each do |type|

    scope type, -> { where(type: "#{type}") }

  end



  attribute :tiers, Plan::Channel::Tiers::Type.new

end

Constant CALCULATION_METHODS is defined as a frozen array of strings and represents how the channel should be calculated. At the beginning everything looks pretty good.


class Plan::CreatePlanForm < BaseForm

  # …



  collection :channels, populator: ChannelsPopulator do

    property :type

    property :rate, type: ::Commissions::Types::Percentage

    property :calculation_method



    validation with: { form: true } do

      required(:type) { filled? & included_in?(Plan::Channel::TYPES) }

      required(:calculation_method) { filled? & included_in?(Plan::Channel::CALCULATION_METHODS) }

    end



    collection :tiers, populate_if_empty: Plan::Channel::Tier do

      property :rate, type: ::Commissions::Types::Percentage

      property :amount, type: ::Commissions::Types::Currency

    end

  end



  # …

end

But suddenly…


class Commission::Calculator::SlidingScale < Commission::Calculator

  # …



  def total_cumulative_amount

    case channel.calculation_method

    when 'sliding_scale_net_origination_fee'

      group.net_origination_fee_amount

    when 'sliding_scale_origination_volume'

      group.total_amount

    end

  end



  # …

end

And again …


class Commission::Calculator

  # …



  class << self

    def calculator(preimage)

      case preimage.channel.calculation_method

      when 'sliding_scale_net_origination_fee',

           'sliding_scale_origination_volume'

        Commission::Calculator::SlidingScale

      when 'flat'

        Commission::Calculator::Flat

      when 'spread'

        Commission::Calculator::Spread

      end

    end

  end



  # …

end

This really looks awful to me. If we change values in CALCULATION_METHODS there is a big chance that we will simply forget to update them in a case-when operators. It would be better to avoid using such unsafe code.

Primitive obsessions are born in moments of weakness. "Just a field for storing some data!" the programmer claimed. Creating a primitive field is so much easier than making a whole new class, right? And so it was done. Then another field was needed and added in the same way.

Primitives are often used to "simulate" types. So instead of a separate data type, you have a set of numbers or strings that form the list of allowable values for some entity. Easy-to-understand names are then given to these specific numbers and strings via constants, which is why they are spread far and wide.

The first approach is to refactor CALCULATION_METHODS constant to four independent constants, so we can arrive at something like this.


class Plan::Channel < ApplicationRecord

  CALCULATION_METHOD_SLIDING_SCALE_NET_ORIGINATION_FEE = 'sliding_scale_net_origination_fee'.freeze

  CALCULATION_METHOD_SLIDING_SCALE_ORIGINATION_VOLUME = 'sliding_scale_origination_volume'.freeze

  CALCULATION_METHOD_FLAT = 'flat'.freeze

  CALCULATION_METHOD_SPREAD = 'spread'.freeze



  CALCULATION_METHODS = [

    CALCULATION_METHOD_SLIDING_SCALE_NET_ORIGINATION_FEE,

    CALCULATION_METHOD_SLIDING_SCALE_ORIGINATION_VOLUME,

    CALCULATION_METHOD_FLAT,

    CALCULATION_METHOD_SPREAD

  ).freeze



  # …

end



# …



class Commission::Calculator

  # …



  class << self

    def calculator(preimage)

      case preimage.channel.calculation_method

      when Plan::Channel::CALCULATION_METHOD_SLIDING_SCALE_NET_ORIGINATION_FEE,

           Plan::Channel::CALCULATION_METHOD_SLIDING_SCALE_ORIGINATION_VOLUME

        Commission::Calculator::SlidingScale

      when Plan::Channel::CALCULATION_METHOD_FLAT

        Commission::Calculator::Flat

      when Plan::Channel::CALCULATION_METHOD_SPREAD

        Commission::Calculator::Spread

      end

    end

  end



  # …

end



But, again, this code does not look good to me. Another approach would be to refactor this code with AR Enum API, but we will do it in another way.

Lets try to use Attributes API again. We have calculation_method column in plan_channels that is String.

We should add it as attribute to Plan::Channel model and define type.


class Plan::Channel < ApplicationRecord

  TYPES = %w(

    direct

    broker

    correspondent

    bdr

    non_funded_origination

  ).freeze



  self.inheritance_column = nil



  belongs_to :plan



  TYPES.each do |type|

    scope type, -> { where(type: "#{type}") }

  end



  attribute :tiers, Plan::Channel::Tiers::Type.new

  attribute :calculation_method, Plan::Channel::CalculationMethod::Type.new

end


require 'dry-initializer'



class Plan::Channel::CalculationMethod

  extend Dry::Initializer



  FLAT = 'flat'.freeze

  private_constant :FLAT



  SPREAD = 'spread'.freeze

  private_constant :SPREAD



  SLIDING_SCALE_NET_ORIGINATION_FEE = 'sliding_scale_net_origination_fee'.freeze

  private_constant :SLIDING_SCALE_NET_ORIGINATION_FEE



  SLIDING_SCALE_ORIGINATION_VOLUME = 'sliding_scale_origination_volume'.freeze

  private_constant :SLIDING_SCALE_ORIGINATION_VOLUME



  SLIDING_SCALE = [

    SLIDING_SCALE_NET_ORIGINATION_FEE,

    SLIDING_SCALE_ORIGINATION_VOLUME

  ].freeze

  private_constant :SLIDING_SCALE



  param :value, proc(&:to_s)



  def self.values

    [FLAT, SPREAD, SLIDING_SCALE].flatten

  end



  def to_s

    value.to_s

  end



  values.each do |v|

    define_method "#{v}?" do

      value == v

    end

  end



  def sliding_scale?

    SLIDING_SCALE.include?(value)

  end



  class Type < ActiveModel::Type::ImmutableString

    def cast(value)

      Plan::Channel::CalculationMethod.new(value)

    end



    def serialize(value)

      case value

      when Plan::Channel::CalculationMethod

        value.to_s

      else

        super

      end

    end

  end

end

Plan::Channel::CalculationMethod became a Value Object. What did the developer gain from refactoring?

  • Instead of a set of primitive values, the programmer has a full-fledged class with all the benefits that object-oriented programming has to offer (typing data by class name, type hinting, etc.).

  • There's no need to worry about data validation, as only expected values can be set.

  • When CalculationMethod logic extends, it will be collected in one place that's dedicated to it.


class Commission::Calculator::SlidingScale < Commission::Calculator

  def rate

    100 * max_commission_amount / group.net_origination_fee_amount

  end



  def charged_amount

    loan.commissionable_amount || loan.net_origination_fee_amount

  end



  private



  def max_commission_amount

    @max_commission_amount ||= tiers.each_cons(2).sum do |previous, current|

      next 0 if total_cumulative_amount <= previous.amount



      commission_charged_amount(previous, current) * current.rate / 100

    end

  end



  def commission_charged_amount(previous, current)

    group.net_origination_fee_amount * cumulative_amount(previous, current) / total_cumulative_amount

  end



  def cumulative_amount(previous, current)

    [ total_cumulative_amount, current.amount ].min - previous.amount

  end



  def total_cumulative_amount

    if channel.calculation_method.sliding_scale_net_origination_fee?

      group.net_origination_fee_amount

    elsif channel.calculation_method.sliding_scale_origination_volume?

      group.total_amount

    end

  end



  def tiers

    @tiers ||= [

      Plan::Channel::Tier.new(amount: 0, rate: 0)

    ] + channel.tiers + [

      Plan::Channel::Tier.new(amount: Float::INFINITY, rate: channel.tiers.last.try(:rate) || 0)

    ]

  end

end


class Commission::Calculator

  class << self

    def calculate(preimage)

      get(preimage).calculate

    end



    def get(preimage)

      calculator(preimage).new(preimage)

    end



    def calculator(preimage)

      if preimage.channel.calculation_method.sliding_scale?

        Commission::Calculator::SlidingScale

      elsif preimage.channel.calculation_method.flat?

        Commission::Calculator::Flat

      elsif preimage.channel.calculation_method.spread?

        Commission::Calculator::Spread

      end

    end

  end



  attr_reader :preimage



  delegate :group, :loan, :channel, :to => :preimage



  def initialize(preimage)

    @preimage = preimage

  end



  def calculate

    preimage.amount = rate * charged_amount / 100

  end

end

The same should be done with TYPES constant. And after this our model looks dry and clean.


class Plan::Channel < ApplicationRecord

  self.inheritance_column = nil



  belongs_to :plan



  Plan::Channel::Type.values.each do |type|

    scope type, -> { where(type: "#{type}") }

  end



  attribute :tiers, Plan::Channel::Tiers::Type.new

  attribute :calculation_method, Plan::Channel::CalculationMethod::Type.new

  attribute :type, Plan::Channel::Type::Type.new

end

The last thing that should be done is to rename plan_channels#type column. I guess source would be the correct name and after this we have a final version of our model.


class Plan::Channel < ApplicationRecord

  belongs_to :plan



  Plan::Channel::Source.values.each do |source|

    scope source, -> { where(source: "#{source}") }

  end



  attribute :tiers, Plan::Channel::Tiers::Type.new

  attribute :calculation_method, Plan::Channel::CalculationMethod::Type.new

  attribute :source, Plan::Channel::Source::Type.new

end

Isn't this over-engineering? A whole new class instead of a string? Really? The answer to such a question is always context-dependent, but I rarely find that it is over-engineering.

You may argue that 15 minutes is a lot, compared to the zero minutes it would take you if you'd 'just' use a string. On the surface, that seems like a valid counter-argument, but perhaps you're forgetting that with a primitive string, you still need to write validation and business logic 'around' the string, and you have to remember to apply that logic consistently across your entire code base. My guess is that you'll spend more than 15 minutes on doing this, and on the troubleshooting defects that occur when someone forgets to apply one of those rules to a string in some other part of the code base.


Igor
Alexandrov

Sr. Full-Stack Developer / Partner at JetRockets

See what people are saying

Rectangle 100 x 175ff7bf cca2 43f0 a34f e04dc8340ed6
Reddit r/ruby
Thread about this article on Reddit.
Rectangle 100 x 2a675e6e fbef 4b86 8c5d 3939c059dfdb
Ruby Weekly
Included in the Issue#387

Explore more of JetRockets