Ruby string literals VS Value objects. Overengineering?

A guide on how to get rid of string literals in Ruby with Rails 5 Attributes API


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

CTO 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