Feedback/suggestions on the snippet with iterators

Ruby on Rails
1

Hello,

I’m looking to improve the following piece of code unless it looks good for you? :smile:

Briefly, there is a list of line_items, each line_item contains sellable instance one of two types, i.e. product_variant or kit, where the kit also contains product variants. The function produces combined variants with quantities.

Thank you in advance!

2

Would something like this be better?
Main thing was just breaking each action into its own method and using #inject to created the hash from a array.

Of course untested :frowning:

def variants_quantities
  line_items.inject({}) do |variants, line_item|
    variants.merge(variant_hash(line_item)) if line_item.sellable.variant?  
    variants.merge(content_hash(line_item)) if line_item.sellable.kit?
  end
end

def variant_hash(line_item)
  line_item.sellable || 0
  { line_item.sellable => line_item.sellable + line_item.quantity }
end

def content_hash(line_item)
  line_item.sellable.content.inject({}) do |variants, content|
    variants.merge(calculate_content(content, line_item))
  end
end

def calculate_content(content, line_item)
  content.variant ||= 0
  { content.variant => content.variant + (content.quantity * line_item.quantity) }
end
3

Thank you @guillec

while the original method is become greatly simpler now, the logic spreading across additional methods doesn’t feels obvious anymore, I’ll probably leave it mostly as is, with only replacement is each_with_object instead of each which allows to build variants from inside the block:

 def variants_quantities
    line_items.each_with_object({}) do |line_item, variants|
      if line_item.sellable.variant?
        variants[line_item.sellable] ||= 0
        variants[line_item.sellable] += line_item.quantity
      elsif line_item.sellable.kit?
        line_item.sellable.contents.each do |content|
          variants[content.variant] ||= 0
          variants[content.variant] += content.quantity * line_item.quantity
        end
      end
    end
  end
4

Just got one more suggestion from colleague, we can reduce two more lines by using Hash.new(0) instead of {}, so, i.e.:

def variants_quantities
  line_items.each_with_object(Hash.new(0)) do |line_item, variants|
    if line_item.sellable.variant?
      variants[line_item.sellable] += line_item.quantity
    elsif line_item.sellable.kit?
      line_item.sellable.contents.each do |content|
        variants[content.variant] += content.quantity * line_item.quantity
      end
    end
  end
end
5

I have two questions:

  1. Do you need to do this kind of select (the two if tests) anywhere else?
  2. Do you need to use code for each branch elsewhere?

If ‘yes’ for #1, you might consider setting up a “Status” class to make this clearer… then you could say something like:

if line_item.variant?
...
elsif line_item.kit?
...
end

Not a huge change for sure, but a bit less wordy.

If ‘yes’ for #2, consider having a method for each of the differen actions. Makes your if statement and thus the method clearer and makes the code reusable, if needed. It might look something like this:

def variants_quantities
...
    if line_item.variant?
      variants << update_variant(line_item)
    elsif line_item.kit?
      variants << update_key(line_item)
    end
...
end    

def update_variant(line_item)
  variants[line_item.sellable] ||= 0
  variants[line_item.sellable] += line_item.quantity
  variants
end

def update_kit(line_item)
  line_item.sellable.contents.each do |content|
    variants[content.variant] ||= 0
    variants[content.variant] += content.quantity *     line_item.quantity
 end
variants
end

The book “Confident Ruby” by Avdi Grimm is really helpful for stuff like this.

6

Thank you @JESii, I had the similar thoughts on further improvements, but decided to leave for the latter considerations since the answer to both question is “No”.

Another idea was to use something like line_item.has_nested_variants? and then line_item.nested_variants, so, we could have it rely on the behavior rather then type.