1

We're implementing a service for creating a subscription where the subscription may be linked to an order. Quick background: the purpose of this service is to allow the customer to receive the item she's subscribing to as early as possible. The solution we chose is to create a separate order for the first item before her "real" subscription starts.

Here's a snippet of the service class. I shortened the name of the directive method to remove extraneous details from the snippet:

class Subscription
  belongs_to :order
end

class SubscriptionCreator
  def call
    # ...
    return directive if directive.is_a?(String)

    # ...

    subscription = create_subscription(order: directive)
    # ...
  end

  private

  def create_subscription(order: nil)
    subscription = Subscription.new

    # ...

    subscription.order = order if order

    subscription.save!

    subscription
  end

  def directive
    # Return values:
    #   * Successful: the order created.
    #   * Error: An error message string.
    #   * Not applicable: nil.
  end
end

Here are the main parts of the service:

  1. SubscriptionCreator#call is the entry point.
  2. SubscriptionCreator#directive creates the order if applicable.
  3. SubscriptionCreator#create_subscription creates the subscription and ties it to the order if one was created.

I felt bothered about the order argument of create_subscription since create_subscription can directly call the directive method. So I proposed this change:

def call
  # ...
  return directive if directive.is_a?(String)

  # ...

  # No longer has order argument.
  subscription = create_subscription
  # ...
end

def create_subscription
  subscription = Subscription.new

  # ...

  subscription.order = directive if directive

  subscription.save!

  subscription
end

However, the author of the service prefers the original code. He argues that it's best if create_subscription doesn't know the logic for creating the order, that is, it doesn't know about the directive method. That logic he says should belong in the call method.

He argues that with the original code, it would be easier to move create_subscription somewhere else because it would contain no domain specific logic. He adds that keeping the lower level layers free from domain logic would make methods more modular and therefore more reusable and easier to extract.

On my part, I think that removing the argument ensures that create_subscription only has one source of information for the created order. In the original code, there's the risk that the order passed to create_subscription is not the same as the result of directive.

So, what are the consequences of keeping/removing the argument from the private method?

  • how are you proposing to uncouple (cancel) a subscription and remove it from an order? – Brandt Solovij Aug 15 '15 at 6:58
  • I'm not sure how removing a subscription from the order is related to the question. Can you clarify? To answer your question: that action would happen in a separate class outside of SubscriptionCreator. It won't be affected by the change I'm proposing inside SubscriptionCreator's class. – gsmendoza Aug 15 '15 at 22:13
  • Gotcha (had to ask) - why pair the "order" action to a subscription? – Brandt Solovij Aug 16 '15 at 18:13
  • I have just updated the description. Copied here: the purpose of the service is to allow the customer to receive the item she's subscribing to as early as possible. The solution we chose is to create a separate order for the first item before her "real" subscription starts. – gsmendoza Aug 16 '15 at 23:21
2

Your Subscription is associated with 0 or 1 Orders. Where those Orders come from doesn't matter to the Subscription, so passing an Order to create_subscription seems reasonable.

In the first snippet, the business of retrieving or creating the appropriate Order is separated from the business of creating the Subscription. In the second, retrieving/creating the Order is embedded in creating a Subscription, and I don't see what you gain by that. If anything, it makes the code harder to change: what if you change the way you retrieve Orders, and what if there were more than one way to retrieve an Order?

The first snippet separates the Order-retrieval business from the Subscription-creation business, and that separation of concerns would make the code easier to maintain, I think.

| improve this answer | |
  • Thanks for the reply. I think the main benefit of embedding the retrieval of order in create_subscription is to ensure that the method has only one source of information for the created order. I updated the description to include this explanation. – gsmendoza Aug 15 '15 at 22:10
  • I see. Then I suppose it depends on the nature of the relationship between Subscription and Order. When would it be invalid to associate an Order with a Subscription? And in the first snippet, when would the Order passed to create_subscription be different from the one in directive? Are you thinking some programmer might get a reference to a different Order in call and pass that to create_subscription instead? Separation of concerns is pretty important, so I think it's worth asking these questions. – mat_noshi Aug 16 '15 at 18:17
  • Right now, create_subscription is only called once, so the risk of passing it a different value than the one submitted by directive is low. However, my preference is to treat the private methods of a class as belonging to a single layer, because the methods all have access to the information the class provides. I think separation of concerns is better done on the class level, where the boundaries of the concerns are clearer. The ideal solution for me would be to extract create_subscription to its own class in order to make the separation of concerns more explicit. – gsmendoza Aug 16 '15 at 23:35
  • Yes, you could do that too -- but either way, I still think the Order retrieval should not be in the same function as the Subscription creation. For simplicity, a function should only do one thing, but if it's retrieving an Order as well as creating a subscription, it's doing two things. A caller to create_subscription might not know that it was also retrieving an Order. If that retrieval involved, say, a database call, you could accidentally end up with a lot of unwanted DB hits that compromise performance. – mat_noshi Aug 17 '15 at 0:21
  • Yeah, that makes sense.. Anyway, thanks for elaborating on the tradeoffs in keeping/removing the argument. It gave me a broader understanding of the tradeoffs for this issue :) – gsmendoza Aug 17 '15 at 1:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.