Disposable Object

So we're running an ecommerce system, and we've got a controller action that handles when orders are placed.

As part of placing an order, a couple of email notifications are sent. There's a receipt for the order. And then there's a followup email which suggests some other products the shopper might like based on what they just purchased.

Both of these emails are sent using a service object of the EmailNotifier class.

require "ostruct"
require "./email_notifier"

class OrderController
  def initialize
    @order = OpenStruct.new(number: 23)
  end

  def current_user
    OpenStruct.new(name: "Bob Example")
  end

  def create
    # ...do order processing...

    @notifier = EmailNotifier.new

    @notifier.send_receipt(current_user, @order)
    @notifier.send_related_products(current_user, @order)
  end
end

Our store is successful, and we steadily accumulate customers. As we do, a nagging bug develops. Every month we send out a promotional email newsletter to every customer who hasn't opted-out. We're not getting the number of email views and click-throughs we'd expect given the size of our database. When we dig into our SMTP logs, we discover that our promotion-sending job seems to mysteriously stop sending out emails partway through the list of customers.

When we take a look at the batch job that sends out the monthly letters, this is what we see.

This code first creates an EmailNotifier. Then it iterates through each user account. For each one, it uses the notifier to send them a monthly promotion email.

notifier = EmailNotifier.new
User.all.each do |user|
  notifier.send_monthly_promo(user, featured_product_list)
end

Let's dive into the EmailNotifier class to see if it holds the key to what's going on.

This class has methods for each type of email it knows how to send. We can see, for instance, that the send_receipt method expects a user and an order object.

Then it assembles an email subject and body .

Finally, it delegates to an internal method called send_email to do the actual sending. One thing we note here is that there is a keyword argument determining whether this email should be treated as promotional. In this case, it's false.

Let's scroll down to the method for sending monthly promo emails. The bulk of this method is similar in form to the one for sending receipts. But unlike for receipts, this one flags the email as promotional.

When we trace down into the send_email method, we can see that first it puts together to and from email addresses. Then, it uses an Optout model to look up the destination email address and check if that address has been opted-out of promotions.

Next there's a conditional. If this is a promotional email and the email address has been opted-out, no send will be performed. Otherwise, it will go ahead.

Looking again at the line that checks if the email has been opted-out, we finally spot the problem. This code uses an instance variable and or-equals assignment to idempotently cache the opt-out value the first time this line of code is run.

Presumably, someone saw that this was a line that could trigger a second database query, and decided to optimize it so it wouldn't re-query every time through. Remember how EmailNotifier is used in the controller: it is used to send two different emails in a row, both to the same email address. In this case, the opt-out value caching works fine, because the customer is the same each time.

But the code for sending monthly newsletters works differently. Here, the same notifier object is used to send emails to many different email addresses.

For this scenario, the opt-out caching is doubly broken. First, because even it it worked as the writer presumably intended, it would apply the first user's email preferences to all customers. But since it's using an ||= operator with boolean values, it has an even more subtly incorrect behavior. It will work properly as long as it processes users who have not opted-out. Each false value will be treated as if the variable is unset, and it will be re-set to false over and over again.

But as soon as it hits a user who has opted-out, that true value will lodge in the @opted_out instance variable, and now that it is truthy no other value will budge it out. As a result, every customer processed after the first opted-out customer will also be treated as if they had opted out.

require "./optout"
require "./mailotron"

class EmailNotifier
  def send_receipt(user, order)
    subject = "Here's your receipt"
    body    = <<~EOF
    Hi #{user.name},

    Here's your receipt for order ##{order.number}:
    ...blah blah blah...
    EOF
    send_email(user, subject, body, promotional: false)
  end

  def send_related_products(user, order)
    subject = "We have some suggestions for you!"
    body    = <<~EOF
    Hey there #{user.name},

    Based on your recent purchases, we have some
    suggestions for you!
    ...yadda yadda yadda...
    EOF
    send_email(user, subject, body, promotional: true)
  end

  def send_monthly_promo(user, promoted_products)
    subject = "Check out what's hot at AcmeCo!"
    body    = <<~EOF
    What's up #{user.name}!

    We've got some exciting products this month that you
    won't want to miss!
    ...etc etc etc...
    EOF
    send_email(user, subject, body, promotional: true)
  end

  def send_email(user, subject, body, promotional:)
    to          = user.email
    from        = "AcmeCo <noreply@example.com"

    @opted_out ||= Optout.find(email: to).opted_out?

    unless promotional && @opted_out
      Mailotron.send(to: to,
                     from: from,
                     subject: subject,
                     body: body)
    end
  end
end

There are plenty of code smells we could ding this code for: a query to a separate table buried deep in logic. Some possibly premature optimization. A naive use of the ||= conditional assignment operator.

But there's a deeper language design factor that allowed this bug to happen in the first place. It's a feature that is increasingly seen by many in the software industry as a fundamental flaw in object-oriented languages. It's the fact that objects have mutable state. When we have objects that can change themselves as a side effect of performing their roles, there's an infinite possibility for objects to behave in unpredictable ways. With mutable state, we can't rely on the same method call with the same arguments having the same outcome every single time. There's always the chance that, like with this example here, some change in state will unintentionally "bleed over" from an earlier operation to a later one.

There are some people who feel that, because of this flaw, we should either abandon stateful languages like Ruby for languages based on immutable data structures such as Haskell or Clojure. There are others who are seeking to bring immutable data structures and coding styles into languages such as Ruby.

Both of these are valid directions that I think are well worth exploring. But sometimes project constraints don't allow for a radical restructuring based on immutability principles, let alone a change in language. And the truth is, I believe that mutable vs. immutable isn't an either/or, black-or-white choice.

In my experience, we can often get 80% of the benefits of immutability not by removing all changeable state, but by minimizing the lifetime of stateful objects. Another way of saying this is to embrace disposable objects.

To treat EmailNotifier as a disposable object back in the batch mailing code, we inline the notifier variable. Now a new EmailNotifier is created for each iteration. The buggy state-bleeding code is still there, but it doesn't matter anymore, because the objects are thrown away before it can cause any trouble.

User.all.each do |user|
  EmailNotifier.new.send_monthly_promo(user, featured_product_list)
end

In order to avoid any future problems, we decide to enforce the use of EmailNotifier as a disposable object. To do that, we add a guard clause to the send_email method that checks a flag variable to see if this object has already been used to send an email. At the bottom of the method, we ensure that the flag is set to true.

require "./email_notifier"

class EmailNotifier
  # ...

  def send_email(user, subject, body, promotional:)
    fail "EmailNotifier cannot be reused!" if @is_used
    to          = user.email
    from        = "AcmeCo <noreply@example.com"

    @opted_out ||= Optout.find(email: to).opted_out?

    unless promotional && @opted_out
      Mailotron.send(to: to,
                     from: from,
                     subject: subject,
                     body: body)
    end
  ensure
    @is_used = true
  end
end

This change causes other uses of EmailNotifier to start blowing up, including the one in the OrderController.

require "./controller"
require "./email_notifier2"

controller = OrderController.new

controller.create # ~> RuntimeError: EmailNotifier cannot be reused!

# ~> RuntimeError
# ~> EmailNotifier cannot be reused!

To fix this we inline the EmailNotifier construction in the controller method.

require "ostruct"
require "./email_notifier"

class OrderController
  def initialize
    @order = OpenStruct.new(number: 23)
  end

  def current_user
    OpenStruct.new(name: "Bob Example")
  end

  def create
    # ...do order processing...

    EmailNotifier.new.send_receipt(current_user, @order)
    EmailNotifier.new.send_related_products(current_user, @order)
  end
end

This code now feels a bit clunky. But instead of interpreting that as a sign that we're headed in the wrong direction, we can lean in and let it guide us to a better and more granular object decomposition.

The idea of disposable objects goes hand-in-hand with using smaller, single-purpose objects. As we find more time to refactor, we can move from an EmailNotifier service object to a collection of smaller process objects, such as we saw in Episode #331, each one representing the act of sending a single email notification. In line with the notification-oriented naming convention we explored in that episode, rather than commanding these objects to send, we notify them that they are free to proceed with their purpose.

require "ostruct"
require "./email_notifier"

class OrderController
  def initialize
    @order = OpenStruct.new(number: 23)
  end

  def current_user
    OpenStruct.new(name: "Bob Example")
  end

  def create
    # ...do order processing...

    ReceiptNotification.new(current_user, order: @order).proceed
    RelatedProductsNotification.new(current_user, order: @order).proceed
  end
end

Disposable objects are like disposable paper plates: we don't have to worry about accumulating a sticky, hard-to-scrub mess of state in them, because we're never going to reuse them. We use them once and then we throw them away. The next time you're fighting bugs caused by accumulated state, see if there's a way to reduce the lifetime of the objects involved. You may find it's cheaper to simply discard old, smelly state than it is to prevent it from building up in the first place. Happy hacking!