The first thing that I noticed that could be improved is to use
find_each instead of
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
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
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")
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:
invalid_user_discounts.find_each do |user_discount|
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:
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|
Rails allows us to merge the two tables
DiscountSegment and as well to select just the fields from that query that we need, in this case:
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