Avoid Putting Logic in Map Blocks

As Rubyists, we write map blocks all the time. These blocks tend to pick up logic that belongs elsewhere. Consider this innocent looking piece of code.

class ShoppingCart
  # other methods

  # SMELLY
  def sub_totals
    items.map do |item|
      item.base_cost + item.bonus_cost
    end
  end
end

If we look closely we can notice a few different code smells:

1. The caller suffers from Feature Envy because it cares a lot about properties on the item and wants to take over item-related responsibilities.
2. Conversely, Item seems to be an Anemic Model that holds data but none of the operations that are done to that data.

Move logic to items being iterated

The body of the block is code that is going to be applied to each item. What if instead, Item owned that logic? This gives Item a richer set of domain operations and keeps related item-related logic in one place.

class Item
  # other methods

  def total_cost
    base_cost + bonus_cost
  end
end

Now we’ve named this domain operation and it has a single source of truth. Our calling code is higher-level and isn’t so tightly coupled to Item internals. As a nice bonus in this case, we get to use symbol to proc syntax.

def sub_totals
  items.map(&:total_cost)
end

Complex block

What about situations where a block has more complicated logic and maybe interacts with other objects? Odds are that most of the logic in there probably still wants to live on Item. After all, the whole point of a map block is applying logic to each of the individual values in the array. If you’re applying logic to an object, that object probably wants to own that behavior.

# SMELLY
def sub_totals
  items.map |item|
    if coupon.applies_to_id == item.id
      (item.base_cost + item.bonus_cost) * coupon.percent_off
    else
      item.base_cost + item.bonus_cost
    end
  end
end

This is still incredibly item-centric. See how many times the item variable is repeated! But the block also introduces a coupon. That’s OK. An item probably wants to own the logic for calculating its total cost, including applying a coupon. We can move that logic to a method on Item and pass the coupon in as an argument.

sub_totals = items.map { |item| item.total_cost(coupon: coupon) }

Avoid extracting to a private method

When faced with the complex block, your first reaction might be to extract the logic out to a private method on the ShoppingCart. While this might make code easier to read, it still suffers from all the same code smells. Put that logic on the instances being iterated instead.

class ShoppingCart
  # other methods

  # STILL SMELLY
  def sub_totals
    items.map { |item| sub_total_for(item) }
  end

  private

  def sub_total_for(item)
    if coupon.applies_to_id == item.id
      (item.base_cost + item.bonus_cost) * coupon.percent_off
    else
      item.base_cost + item.bonus_cost
    end
  end
end