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
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)
end
else
current_item = line_items.create(product: product, quantity: quantity)
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

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?

• 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. – Ryan.lay Jul 16 '16 at 9:09

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

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
# [...]

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