Rails service for a series of calculations

I'm writing a Rails service in which a series of operations are performed on a record:

class RecordBaseAmountService

self.build
new
end

def call(record, subrecord)
record = calculate_base_amount(record, subrecord)
record = pro_rate_record(record, subrecord) if subrecord.is_prorated?

return record
end

private

def calculate_base_amount(record, subrecord)
if subrecord.type == 0
base_amount = 10
elsif subrecord.type == 1
base_amount = 20
else
base_amount = 30
end

record.base_amount = base_amount
record.audit_field += "=>base amount calculated"
record
end

record.base_amount = record.base_amount*30
record
end

def pro_rate_record(record, subrecord)
record.pro_rated_amount = record.base_amount*subrecord.pro_rate
record.base_amount = record.base_amount - record.pro_rated_amount
record.audit_field += "=>prorated"
record
end
end


The Record and Subrecord classes are basic Activerecord classes with validations, scopes, callbacks, and associations. Basically almost all of my business logic is farmed out to services and my models only deal with data integrity.

class Record < ActiveRecord::Base
has_many :subrecords
validates :base_amount, presence: true
around_update :cache_in_subrecord

private

def cache_in_subrecord
subrecord = self.subrecord
self.audit_field += "=>subrecord cached " + Time.now.utc.to_date.to_s
subrecord.cached_attribute_field = (subrecord.cached_attribute_field || {}).merge(get_cached_attributes)

yield

subrecord.save
end

def get_cached_attributes
{
record_id: self.id,
base_amount: self.base_amount,
audit_field: self.audit_field,
time: Time.now.utc.to_date
}
end
end

class Subrecord < ActiveRecord::Base
belongs_to :record
validates :type, presence: true
validates :start_date, presence: true
validates :pro_rate, presence: true

def is_prorated?
same_month? && not_first_day?
end

def same_month?
self.start_date.month == Date.today.month
end

def not_first_day?
self.start_date.day > 1
end
end


and the service is called from multiple places including controllers and rake tasks. In one particular rake task I call the service above with the Celluloid gem.

require "celluloid"

class MonthlyRecordWorker
include Celluloid

def update_record(record)
updated_record = RecordBaseAmountService.build.call(record, record.subrecord)
updated_record.save
end

end

def call
record_pool = MonthlyRecordWorker.pool(size: 10)

Record.all.each do |i|
record_pool.async.update_record(i)
end
end

end

namespace :monthly do
end
end


• Thanks for updating the code so that the question is no longer off-topic. However, it is still lacking context and a title that describes what the service does. See How to Ask. – 200_success Jun 20 '16 at 18:43
• Can you tell us more about the record object, or post the definition of that class? Can you also provide some example code for how this service class is used? – Greg Burghardt Jun 20 '16 at 19:12
• @GregBurghardt I've updated the question with the details you've asked for. – Ryan.lay Jun 21 '16 at 1:25

The real threat to thread safety is actually in your RecordBaseAmountService class. This takes two arbitrary objects, a record and sub-record. This logic really should be moved into the Record class to ensure you are modifying the objects you think you are. Consider this case:

record1 = Record.find 3
record2 = Record.find 8
subrecord = record1.subrecord
update_record = RecordBaseAmountService.build.call record2, subrecord


Do you see any bugs here? Any thread safety issues? We pass in record2 but pass in the sub record for record1 -- oops!

Moving this logic into the Record class itself encapsulates this behavior and limits it to the proper objects. This also limits the ability of code outside that Record object to modify data outside its thread.

class Record < ActiveRecord::Base
has_many :subrecords
validates :base_amount, presence: true
around_update :cache_in_subrecord

def calculate_base_amount
if subrecord.type == 0
base_amount = 10
elsif subrecord.type == 1
base_amount = 20
else
base_amount = 30
end

update_audit_field "base amount calculated"
pro_rate_record if subrecord.is_prorated?

base_amount
end

private

base_amount = base_amount * 30
end

def pro_rate_record
pro_rated_amount = base_amount * subrecord.pro_rate
base_amount = base_amount - pro_rated_amount
update_audit_field "prorated"
end

def cache_in_subrecord
subrecord = self.subrecord
update_audit_field "subrecord cached " + Time.now.utc.to_date
subrecord.cached_attribute_field = (subrecord.cached_attribute_field || {}).merge(get_cached_attributes)

yield

subrecord.save
end

def get_cached_attributes
{
record_id: self.id,
base_amount: self.base_amount,
audit_field: self.audit_field,
time: Time.now.utc.to_date
}
end

def update_audit_field(message)
audit_field += "=>" + message
end

end


The RecordBaseAmountService is entirely unneeded.

The MonthlyRecordWorker is still useful, but could use a simple change:

class MonthlyRecordWorker
include Celluloid

def update_record(record)
record.calculate_base_amount
record.save
end

end


Nothing else needs to change. Just encapsulate the base amount calculation to limit the chances that objects get modified outside the current thread.

Some nitpicks:

• Use 2 spaces for indentation. I see 3 and 4 spaces.

• Put spaces around all operators:

record.base_amount = record.base_amount*30


Becomes:

record.base_amount = record.base_amount * 30

• No need to convert a date object to a string when updating the audit message. The string concatenation operation will do that automatically

self.audit_field += "=>subrecord cached " + Time.now.utc.to_date.to_s

• No need for a build method if it just delegates to new

worker = MonthlyRecordWorker.new
worker.call


Or:

MonthlyRecordWorker.new.call


Suggestions:

• Create an update_audit_field method instead of concatenating strings and prefixing it with => constantly (in Record):

def update_audit_field(message)
audit_field += "=>" + message
end

• Thank you for the well thought out review Greg. Very much appreciated, especially section about the minor nitpicks. I'd like to explain my thought process with my code structure. The service is actually one of about a dozen services whose logic relates to this one model, so unfortunately splicing my logic into component services is the only way to keep the code organized and sane. – Ryan.lay Jun 22 '16 at 16:02
• With rapidly changing specs and requirements, the services sometimes require additional functionality which I would add with dependency injection, hence the call to Service.build.call. That way, I can add additional dependencies to any one service without affecting code anywhere else. – Ryan.lay Jun 22 '16 at 16:03
• I'm curious about your first point. If I ensure that the objects passed in are correct then the code is thread safe? – Ryan.lay Jun 22 '16 at 16:08
• That is correct. But now you are assuming that the caller is passing in the correct object. Furthermore, this logic only operates on one Record and its Subrecord. Architecturally this logic belongs in the Record class to properly encapsulate that behavior. – Greg Burghardt Jun 22 '16 at 16:10
• Thank you. What if the call to the service is moved to the Model? That should ensure the objects passed are correct would it not? – Ryan.lay Jun 22 '16 at 16:12