Rails service for a series of calculations

Question

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 = adjust_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
 def adjust_base_amount(record, subrecord)
 record.base_amount = record.base_amount*30
 record.audit_field += "=>base amount adjusted"
 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
class MonthlyTaskService
 def call
 record_pool = MonthlyRecordWorker.pool(size: 10)
 Record.all.each do |i|
 record_pool.async.update_record(i)
 end
 end
end
#lib/tasks/monthly.rake
namespace :monthly do
 desc "Cron tasks"
 task :audit_records => :environment do
 MonthlyTaskService.build.call
 end
end

I'm highly concerned about thread safety, which I have no experience with. Is this code thread safe? Any comments on any improvements to be made?

• 200_success: 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.
• Greg Burghardt: 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?
• Ryan.lay: @GregBurghardt I've updated the question with the details you've asked for.

Answer 1

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"
 adjust_base_amount
 pro_rate_record if subrecord.is_prorated?
 base_amount
 end
 private
 def adjust_base_amount
 base_amount = base_amount * 30
 update_audit_field "base amount adjusted"
 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
  

• Ryan.lay: 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: 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: I'm curious about your first point. If I ensure that the objects passed in are correct then the code is thread safe?
• Greg Burghardt: 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.
• Ryan.lay: 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?