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.