Excerpt
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:
The caller suffers from Feature Envy because it cares a lot about properties
on the item and wants to take over item-related responsibilities.
Conversely, Item seems to be an Anemic Model that holds data but none
of the operations that are done to that data.
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
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:
The caller suffers from Feature Envy because it cares a lot about properties
on the item and wants to take over item-related responsibilities.
Conversely, Item seems to be an Anemic Model that holds data but none
of the operations that are done to that data.
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
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) }
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
If you enjoyed this post, you might also like: