In Progress
Unit 1, Lesson 21
In Progress

Decorator Transparency

Video transcript & code

In the previous episode we talked about the Decorator pattern. One of the most important properties of a decorator is transparency. This refers to the idea that a decorated object should be a drop-in replacement for the original, un-decorated object.

Let's take another look at our AuditedAccount example from the last episode.

require 'delegate'
class AuditedAccount < SimpleDelegator
  def initialize(account, audit_log)
    super(account)
    @audit_log = audit_log
  end

  def deposit(amount)
    super
    @audit_log.record(number, :deposit, amount, balance_cents)
  end

  def withdraw(amount)
    super
    @audit_log.record(number, :withdraw, amount, balance_cents)
  end
end

On the face of it, it looks as if it has the property of transparency. We can take an audited bank account and do all the things we could do with a vanilla bank account. We can deposit funds, withdraw funds, check the balance, and ask the object to describe itself.

require "./bank_account"
require "./audit_log"
require "./audited_account"

account = AuditedAccount.new(BankAccount.new(123), AuditLog.new)
account.deposit 1000
account.withdraw 500
account.balance_cents           # => 500
account.inspect                 # => "#<BankAccount 123 ($5.00)>"
# >> 2014-03-26 12:09:22 -0400 #123 deposit 10.00 (10.00)
# >> 2014-03-26 12:09:22 -0400 #123 withdraw 5.00 (5.00)

Now let's say we have a preexisting method in our code that does two things: makes a withdrawal, and notifies the customer of their new balance.

class Customer
  def notify(message)
    # ...notify the user somehow...
  end
end

def withdraw_and_notify(customer, account, amount)
  new_balance = account.withdraw(amount)
  customer.notify("Your new balance is %.2f" [new_balance / 100.0])
end

When we pass an audited account into this method, to our surprise, it blows up:

require "./bank_account"
require "./audit_log"
require "./audited_account"
require "./withdraw_and_notify"

account = AuditedAccount.new(BankAccount.new(123), AuditLog.new)
customer = Customer.new
account.deposit(1000)
withdraw_and_notify(customer, account, 500)
# ~> /home/avdi/Dropbox/rubytapas/198-more-decorator/withdraw_and_notify.rb:10:in `withdraw_and_notify': undefined method `/' for nil:NilClass (NoMethodError)
# ~>    from -:9:in `<main>'
# >> 2014-03-26 12:54:07 -0400 #123 deposit 10.00 (10.00)
# >> 2014-03-26 12:54:07 -0400 #123 withdraw 5.00 (5.00)

The reason for this failure is that the seemingly innocent #withdraw_and_notify method depends on a property of the #withdraw method that we hadn't drawn attention to before now. When we look at the original implementation, we can see that besides for reducing the amount of the account balance, it also implicitly returns the new account balance.

def withdraw(amount)
  @balance_cents -= amount
end

We can see this if we withdraw funds from a bank account and look at the return value. But when we wrap the account in an AuditedAccount decorator and make another withdrawal, the return value is nil.

require "./bank_account"
require "./audit_log"
require "./audited_account"
require "./withdraw_and_notify"

account = BankAccount.new(123)
account.deposit(1000)
account.withdraw(300)      # => 700

account = AuditedAccount.new(account, AuditLog.new)
account.withdraw(200)           # => nil
# >> 2014-03-26 12:59:34 -0400 #123 withdraw 2.00 (5.00)

Looking at the AuditedAccount source code, we can see why this is the case. The last statement in the method is a send to @audit_log.record. This method performs a printf, which always returns nil.

def withdraw(amount)
  super
  @audit_log.record(number, :withdraw, amount, balance_cents)
end

So our seemingly transparent decorator turns out to only be translucent.

We can fix this by explicitly assigning the return value of the super call to a variable, and then making that variable the last statement in the method.

def withdraw(amount)
  new_balance = super
  @audit_log.record(number, :withdraw, amount, balance_cents)
  new_balance
end

Another way to do this is to use #tap to preserve the original return value. We send #tap to the result of super, and wrap the rest of the method in the block given to #tap. The return value of the block will be ignored, and the return value of super will be returned from the method.

def withdraw(amount)
  super.tap do
    @audit_log.record(number, :withdraw, amount, balance_cents)
    new_balance
  end
end

I slightly prefer this approach. If we ever add more code, the natural tendency will be to grow the code inside the #tap block, leaving the return value unaffected. In my experience, when using the explicit variable version, it's a little too easy to unthinkingly add code after the variable and once again break the method's return value.

But hang on a second. Let's step back, and reexamine our assumptions.

So far, we've been treating this lack of perfect transparency as a failure on the part of the decorator. But there's another perspective we can look at this from.

We've mentioned the concept of Command/Query Separation in the past. This is the notion that a method should be either a command with side effects, or a query that returns a value, but never both.

The #withdraw_and_notify method violates command/query separation. It expects to use the #withdraw message both as a command—instructing the account to reduce its balance by a given amount—and as a query, returning the new balance. This second responsibility, of returning the new balance, isn't even indicated by the name of the #withdraw method. It's just an accidental feature resulting from the way #withdraw happens to be implemented in the original BankAccount class.

But even if the method return value had been a documented part of the BankAccount interface, I still think that this is a terrific example of how the idea of Command/Query Separation can steer us away from subtle bugs. We originally implemented AuditedAccount in the most obvious way possible: first forward to the delegate method, then on the next line add the extra auditing functionality. It was the lack of Command/Query Separation that caused this obvious implementation to break our existing code.

Let's make the assumption that we do respect CQS in our system. So methods are either commands or queries, not both. As a command method with side effects, we do not expect #withdraw to have a meaningful return value. As such, the original implementation of AuditedAccount now qualifies as transparent, because we no longer consider the return value of a command like #withdraw to be part of its interface. When we look at things from this new point of view, our plan of attack is clear: we need to fix the #withdraw_and_notify method.

We update it to separately withdraw and then get the new balance.

def withdraw_and_notify(customer, account, amount)
  account.withdraw(amount)
  new_balance = account.balance_cents
  customer.notify("Your new balance is %.2f" [new_balance / 100.0])
end

This resolves the issue at hand. And with our resolution to respect CQS in our class interfaces, we reduce the chances of future transparency issues cropping up.

Happy hacking!

Responses