2
\$\begingroup\$

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?

\$\endgroup\$
  • 3
    \$\begingroup\$ 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. \$\endgroup\$ – 200_success Jun 20 '16 at 18:43
  • 1
    \$\begingroup\$ 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? \$\endgroup\$ – Greg Burghardt Jun 20 '16 at 19:12
  • \$\begingroup\$ @GregBurghardt I've updated the question with the details you've asked for. \$\endgroup\$ – Ryan.lay Jun 21 '16 at 1:25
1
\$\begingroup\$

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
    
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Ryan.lay Jun 22 '16 at 16:02
  • \$\begingroup\$ 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. \$\endgroup\$ – Ryan.lay Jun 22 '16 at 16:03
  • \$\begingroup\$ I'm curious about your first point. If I ensure that the objects passed in are correct then the code is thread safe? \$\endgroup\$ – Ryan.lay Jun 22 '16 at 16:08
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Greg Burghardt Jun 22 '16 at 16:10
  • \$\begingroup\$ 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? \$\endgroup\$ – Ryan.lay Jun 22 '16 at 16:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.