Adding product with attributes

Question

I've been working on refactoring a piece of my code that allows a user to add items to their cart. Each product can also be added with different attributes. Thus if somebody adds the same product, with matching attributes it shouldn't be added again. It should just increase the quantity by += X where X is the quantity they've chosen.

Now I feel like it can be refactored even more, but I'm not sure how to deal with that when it comes to for example making it have less queries or be faster. As it's one of the slowest processes on our application.

The add_product gets a parameter containing product_id, line_item_attributes_attributes and quantity. What I do now is check initially if the cart has that product already in the cart. If it does, it checks if the attributes are a complete match if so adds quantity, if not adds it as a new product.

 // Cart.rb
 def add_product(parameters)
 product = Product.find(parameters[:product_id])
 quantity = parameters[:quantity].to_i
 product_attributes = parameters[:line_item_attributes_attributes].try(:to_unsafe_h) || {}
 values = product_attributes.values
 collect_ids = values.map { |attribute| attribute[:product_attribute] } 
 .compact
 .flatten
 .reject(&:empty?)
 attribute_values = collect_ids.map(&:to_i)
 attach_store_to_cart(product.store) if line_items.empty?
 current_item = line_items.where(product_id: product.id)
 if current_item
 existing_item = look_for_exact_item_match(current_item, attribute_values)
 if existing_item
 current_item = line_items.find(existing_item)
 current_item.update_attribute(:quantity, current_item.quantity += quantity)
 else
 current_item = line_items.create(product: product, quantity: quantity)
 add_attributes(current_item.id, attribute_values) if attribute_values.present?
 end
 else
 current_item = line_items.create(product: product, quantity: quantity)
 add_attributes(current_item.id, attribute_values) if attribute_values.present?
 end
 current_item
 end
 def look_for_exact_item_match(similar_items, attribute_values)
 line_item_id = nil
 similar_items.each do |item|
 if item.line_item_attributes.map(&:product_attribute_id).sort == attribute_values.sort
 line_item_id = item.id
 return line_item_id
 end
 end
 line_item_id
 end
 def add_attributes(current_item_id, attributes)
 current_time = Time.current
 columns = 'line_item_id, product_attribute_id, created_at, updated_at'
 values = attributes.map { |attribute| "(#{current_item_id},#{attribute}, TIMESTAMP '#{current_time}', TIMESTAMP'#{current_time}')" }.join(',')
 ActiveRecord::Base.connection.execute("INSERT INTO line_item_attributes (#{columns}) VALUES #{values}")
 end
 def attach_store_to_cart(store)
 update_attribute(:store, store)
 end

I feel like I might be able to improve the checking of attributes, and the collect_ids variable as it's currently doing 3 different things for the array. Which I predict is quite intensive.

I've tried to improve the process already by inserting all the attributes in one query.

I'd be interested to see how this can be improved in terms of efficiency and speed but also in an OOP way. Any feedback?

• Ryan.lay: Can you show us where the Cart.add_product is called? And if you want people to give you meaningful help you should accept the answers to your questions.

Answer 1

You're dealing with a lot of incidental complexity here. Especially extracting the product attributes from the parameters seems to be quite involved. If you have control over the form that's producing those there might be more opportunity for making things easier. As things currently are, I decided to create a factory class to allow us to break things down into smaller methods.

I didn't think deeper about the performance problem but how to structure the code in a more OO way. The main idea is to always construct a new line item and compare that to all existing line items until a match is found. The matching logic can go into the LineItem class. We can further simplify things if the LineItem could also take care about its product attributes. I punted on this and created a serialized attribute, but you might as well set up a proper association for this, maybe with a custom writer method for convenience.

Of course you need good test coverage to do a refactoring like this.

class LineItemFactory
 def self.build(parameters)
 new(parameters).build
 end
 def initialize(parameters)
 @parameters = parameters
 end
 def build
 LineItem.build(
 product: product,
 quantity: quantity,
 product_attributes: product_attributes
 )
 end
 private
 attr_reader :parameters
 def product
 @product ||= Product.find(parameters[:product_id])
 end
 def quantity
 @quantity ||= parameters[:quantity].to_i
 end
 def product_attributes
 @product_attributes ||=
 begin
 product_attributes = parameters[:line_item_attributes_attributes].try(:to_unsafe_h) || {}
 product_attributes.values.map { |attribute| attribute[:product_attribute] }
 .flatten
 .reject(&:empty?)
 .map(&:to_i)
 end
 end
end
class LineItem
 serialize :product_attributes
 delegate :store, to: :product
 def matches?(other)
 self.product_id == other.product_id &&
 self.product_attributes.sort == other.product_attributes.sort
 end
end
class Cart
 # [...]
 def add_product(parameters)
 line_item = LineItemFactory.build(parameters)
 attach_store_to_cart(line_item.store) if line_items.empty?
 existing = line_items.detect { |li| li.matches?(line_item) }
 if existing
 existing.quantity += line_item.quantity
 existing.save
 else
 line_items << line_item
 end
 end
 def attach_store_to_cart(store)
 update_attribute(:store, store)
 end
end