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?

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