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.
