---
title: Avoid Putting Logic in Map Blocks
teaser: The contents of a `map` block should probably be an instance method on each
  item.
tags: web,ruby,domain modeling
author: Joël Quenneville
published_on: 2023-05-26
---

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.

```ruby
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.

[**Feature Envy**]: https://wiki.c2.com/?FeatureEnvySmell
[**Anemic Model**]: https://martinfowler.com/bliki/AnemicDomainModel.html

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

```ruby
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.

```ruby
def sub_totals
  items.map(&:total_cost)
end
```

[symbol to proc]: https://thoughtbot.com/blog/blocks-procs-and-enumerable

## 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._

```ruby
# 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.

```ruby
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.

```ruby
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
```