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