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?

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