Private method with a derivable argument - a code smell?

Question

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?

Answer 1

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.