Jake Douglas

writing code that does stuff

Every day use of the decorator pattern

with 4 comments

At some point I found that my tests were too complicated and hard to maintain. I saw people talking about design patterns, but assumed that they were an Enterprise Thing that I should stay away from. It turns out that design patterns are useful even in vanilla Rails programming and have significantly improved the quality of my code.

James Golick has a great gentle introduction to some of these ideas and why they lead to more easily tested and maintainable code. I thought it was worth sharing some examples that might help the principles click with more people. This post will discuss the decorator, and assumes that you are familiar with some basic concepts like dependency injection.

Lets take an example from our social network application. Users have nicknames and we’re writing some code that lets a user change their nickname. Obviously we need to update the user’s database record with the new nickname, but there are a few other things:

  1. We need to keep a history of which nicknames a person has used. To do this we’ll create a NicknameHistory record when they successfully change their nickname.
  2. A user is only allowed to change their nickname every 28 days. You can imagine that the historical records we’re keeping would allow us to determine whether a user is eligible, but for this example you should take it for granted that the NicknameChangePolicy implements this logic correctly.
  3. We want to send an email confirmation to the user after they successfully update it.

We’ll start with one new class that is going to perform these duties. It will also return a boolean to indicate whether or not the update succeeded.

class NicknameUpdateService
  def initialize(nickname_change_policy = NicknameChangePolicy.new,
                 nickname_history_klass = NicknameHistory,
                 mailer                 = SomeMailer)
    @nickname_change_policy = nickname_change_policy
    @nickname_history_klass = nickname_history_klass
    @mailer                 = mailer
  end

  def update(user, nickname)
    old_nickname = user.nickname

    if @nickname_change_policy.can_proceed?(user)
      user.update_attributes(:nickname => nickname).tap do |was_successful|
        if was_successful
          @nickname_history_klass.create!(:from => old_nickname,
                                          :to   => nickname,
                                          :user => user)

          @mailer.deliver_nickname_change_notification(user)
        end
      end
    else
      false
    end
  end
end

This code fulfills all of the requirements, but theres a lot going on and the behaviors are tied together instead of being re-usable individually. Also, look at the RSpec specification for such a class:

describe NicknameUpdateService do
  context "given the user is eligible to update their nickname" do
    context "and the update succeeds" do
      it "creates a NicknameHistory record"
      it "sends a notification email"
      it "returns true"
    end

    context "and the update fails" do
      it "doesn't create a NicknameHistory record"
      it "doesn't send a notification email"
      it "returns false"
    end
  end

  context "given the user is not eligible to update their nickname" do
    it "doesn't try to update the user record"
    it "doesn't create a NicknameHistory record"
    it "doesn't send a notification email"
    it "returns false"
  end
end

We have to test every conditional branch in the method, so in this case we already have 3 scenarios. Imagine that we wanted to add another condition, that the email confirmation should only be sent if it’s also a Tuesday. The complexity of the tests quickly balloons and they become hard to maintain and extend.

Considering the Single Responsibility Principle, each behavior should probably be factored out into its own class. These classes can then be chained together, each one of them “decorating” the behavior of the class they wrap with its own. We can start with the most basic class that simply updates the user record:

class Nickname::UpdateService
  def update(user, nickname)
    user.update_attributes(:nickname => nickname)
  end
end

Next, we’ll decorate the base class with the creation of a historical record when the update succeeds.

class Nickname::UpdateServiceWithHistory
  def initialize(update_service         = UpdateService.new,
                 nickname_history_klass = NicknameHistory)
    @update_service         = update_service
    @nickname_history_klass = nickname_history_klass
  end

  def update(user, nickname)
    old_nickname = user.nickname

    @update_service.update(user, nickname).tap do |was_successful|
      if was_successful
        @nickname_history_klass.create!(:from => old_nickname,
                                        :to   => nickname,
                                        :user => user)
      end
    end
  end
end

Then we have to send the email.

class Nickname::UpdateServiceWithEmailNotification
  def initialize(update_service = UpdateServiceWithHistory.new,
                 mailer         = SomeMailer)
    @update_service = update_service
    @mailer         = mailer
  end

  def update(user, nickname)
    @update_service.update(user, nickname).tap do |was_successful|
      if was_successful
        @mailer.deliver_nickname_change_notification(user)
      end
    end
  end
end

Finally, the outer-most decorator needs to ensure that none of this takes place unless the user is eligible to change their nickname. This is the class that would actually get called from a controller or similar.

class Nickname::UpdateServiceWithEligibilityAwareness
  def initialize(update_service         = UpdateServiceWithEmailNotification.new,
                 nickname_change_policy = NicknameChangePolicy.new)
    @update_service         = update_service
    @nickname_change_policy = nickname_change_policy
  end

  def update(user, nickname)
    if @nickname_change_policy.can_proceed?(user)
      @update_service.update(user, nickname)
    else
      false
    end
  end
end

There are a number of resulting benefits:

  1. Each piece of behavior can have its own unit tests that only have to consider a maximum of two branches.
  2. Each piece of behavior can be re-used independent of the other behaviors.
  3. Adding new behavior does not require changing the existing behaviors, only the chain of classes. This is made particularly easy since the interfaces are all identical, or in the case of Ruby, just the method signature and the meaning of the return value.

There are a few disadvantages as well, that I consider to be negligible in light of the benefits:

  1. We have to write extra boilerplate to define the additional classes and tests.
  2. The entire set of behavior cannot be understood by reading a single method, and instead requires looking at multiple classes and how they decorate one another. I haven’t had a problem with this, but using a factory might help make it more clear which decorators are being used.

A more diligent tester might create a factory class and a corresponding unit test to verify that the decorators are being chained together correctly. However, I haven’t been bitten by not doing this yet, and we rely on integration tests for verifying behavior that we consider to be critical.

Advertisements

Written by jakedouglas

October 16, 2011 at 6:31 pm

Posted in Code design

4 Responses

Subscribe to comments with RSS.

  1. Deep decoration always reminds me of Jamie Zawinski on regular expressions. You wind up with a bunch of deeply nested methods that are only called from one place. At least, you hope they aren’t, because the bottom one, the one that actually does the real work, is supposed to be protected from doing the wrong thing by all those decorators.

    I think you were better off with that first version. 😦

    Ross Patterson

    October 17, 2011 at 4:50 pm

    • That is if you never need to re-use any of the distinct pieces of behavior somewhere else. The natural progression of this exercise is to build another feature that demonstrates how easily these pieces can be composed for a similar purpose with some of the requirements being different.

      jakedouglas

      October 17, 2011 at 5:24 pm

      • The nickname example is simple enough that using decorators ends up making it more complex and (potentially) just as hard to maintain as the first example. Where decorators really shine is in far more complex examples, where some form of modularity is basically a necessity.

  2. I’am not sure you really have improved your code. Sure you should test your 3 basic services : create an history record, send a notification email and update user record.

    In a real application thoses step will be complex enough to warrent their own class/method anyway. And you’ll unit test them separately. No need for decorator at all.

    But your code here is very specific each time theses 3 very generic services code you just made are dependant of a successfull update of the previous step. This update status has nothing to do with sending email, create DB record or whatever.

    Why not just have a method that call the other 3 ?

    I’am not a ruby expert, in java you’ll could write the chaining like:

    updateNickname(user, userName) && createHistoryRecord(oldNickName, nickName, user) && sendEmail(user);

    you can use the verbose version:

    if (updateNickname(userName) {
    if (createHistoryRecord(oldNickName, nickName, user) {
    sendEmail(user);
    }
    }

    or automatically benefit of exceptions and transactions:

    @Transactional
    void nickNameUpdate(User user, String oldNickName, String newNickName) {
    updateNickName(user, newNickName)
    createHistoryRecord(user, oldNickName, newNickName)
    sendEmail(user)
    }

    That least version also ensure that you not update the user nickname if you failed to create the history record has everything would be rollbacked.

    This is not to say java is better, as I think you can write similar things in ruby, and thus using java is only because I not so good at writting ruby.

    But the key point here :

    The method that send mail to user etc are easier to test : no successfull check of previous test needed. And because in another context we might not have previous step, this is better. This is also better if you decide say to change the order of execution, creating the history record and only after update the user record.

    The code logic is really clear. On one side you have a set of service (send mail, add history record…) and on the other side the workflow logic is entirely code in only one method. This far better stick to single responsability principle.

    If tomorrow we decide to send mail only in case of failure for example, or send it in all cases (with a notification of failure), only one method need to be changed, and not the individual services.

    Last and not least, the code is really easier to read and understand. You say it is not important and that easier to test code is more important.

    I do not agree.

    Here this code is very small, you just written it, and obviously you understand it. Imagine if you didn’t write it and thoses line were messed up with 1 or 2 other million line of code in big application. Having an explicit logic and easier to understand code would really help.

    Second point, it is not easier to test, as I just shown alternate way to code it allow an easy testing and also provide easier understanding and less boilerplate.

    Third point, unit testing is just one small part of testing. It discover only 20-40% of all bugs. It must be coupled with lot of testing methods like integration testing, formal code reviews and beta testing. Increasing your code complexity, reducing its visibility and it boilerplate for an hypothetical easier testing is not as good as it first appear.

    If you really need a pattern here, that’s the command pattern together maybe with command chaining. Not decorator. And it is likely that you do not need a pattern at all. Just write plain and lisible code.

    Theses pattern would bring real value if the number of steps would be unknown and the steps themselves would be configurable and change from run to run.

    We need to learn and master design pattern. And the last and most important part in learning it is to use them only when necessary and as few time as possible.

    Nicolas

    October 18, 2011 at 2:18 am


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: