Re-creating user discounts nightly

I’m calling these two methods on a Worker that is executed every 24 hours at midnight in the background.

The code seems to be a little bit difficult to read and I’m not sure if i’m iterating through the collections in the smartest way.

Is there a more elegant or faster way to iterate on these methods?
Would you suggest any other change?

  # Deletes UserDiscounts when the discount belongs to a segment that the user does not belong to
  def clean_user_discounts
    UserDiscount.all.each do |user_discount|
      user_segment = UserSegment.find_by(user: user_discount.user)
      discount_segment = DiscountSegment.find_by(discount: user_discount.discount)
      user_segment_id = user_segment ? user_segment.segment_id : nil
      discount_segment_id = discount_segment ? discount_segment.segment_id : nil
      user_discount.delete unless discount_segment_id == user_segment_id
    end
  end

  # Creates UserDiscounts when a user belongs to a segment and that segment has a discount
  def create_user_discounts
    UserSegment.all.each do |user_segment|
      DiscountSegment.where(segment: user_segment.segment).each do |discount_segment|
        unless UserDiscount.find_by(discount: discount_segment.discount, user: user_segment.user)
          UserDiscount.create(discount: discount_segment.discount, user: user_segment.user)
        end
      end
    end
  end

The first thing that I noticed that could be improved is to use find_each instead of all.

all will load all the records from the database at once, which can blow up your server. Using find_each instead, you’ll load records in batches.

Also, another thing here is that you’re using heavily object in queries, for instance: UserSegment.find_by(user: user_discount.user) instead of UserSegment.find_by(user_id: user_discount.user_id)

When using object references like that, if the object isn’t loaded rails will perform a query to retrieve the object – therefore this code have a N+1 problem in it – meaning you end up loading those objects (such as user, discount and segment) even though have basically the same info in the parent object already loaded.

So always prefer to ids instead of objects for those type of queries.

If I understood your problem correctly I think you can simplify greatly its performance and syntax by just leveraging ActiveRecord query options. Instead of loading the objects in memory and using ruby to do the verifications for you, you can just instruct ActiveRecord to perform joins and whatnot and just return to you the information that you need.

Here’s how I’d do it:

First, for removing invalid user discounts I’d create a query that return them to me, like so:

class UserDiscount < ApplicationRecord
  belongs_to :user
  belongs_to :discount

  def self.invalid_user_discounts
    UserDiscount.
      joins("INNER JOIN discount_segments ON user_discounts.discount_id = discount_segments.discount_id").
      joins("INNER JOIN user_segments ON user_discounts.user_id = user_segments.user_id").
      where("user_segments.segment_id <> discount_segments.segment_id")
  end
end

See how instead of loading multiple classes I just let the powerful SQL and rails do the job for me? This query isn’t too complicated to understand and does the same job as the previous loop.

Once we have a query that returns invalid discounts, I can use find_each and remove them:

def self.clean_user_discounts
  invalid_user_discounts.find_each do |user_discount|
    user_discount.destroy
  end
end

For creating discounts, I use the same approach, I let AR and SQL do the heavy lifting in this case, and just use the final formatted response to iterate over the results, like so:

def self.create_user_discounts
  query = UserSegment.
    joins("INNER JOIN discount_segments ON user_segments.segment_id = discount_segments.segment_id").select(:id, :discount_id, :user_id)


  query.find_each do |user_and_discount_segment|
    UserDiscount.find_or_create_by(
      discount_id: user_and_discount_segment.discount_id,
      user_id: user_and_discount_segment.user_id)
  end
end

Rails allows us to merge the two tables UserSegment and DiscountSegment and as well to select just the fields from that query that we need, in this case: user_id, discount_id – making the creation of UserDiscount much easier.

Finally, you can see the usage of find_or_create_by – this is provided by Rails as well, so we don’t have to issue two commands.

full source code: https://gist.github.com/jonduarte/c54146cb7e00f0045193aad739301c13