Ok not to use all parameters in constructor? - Revisited

Question

This class injects all dependencies in the constructor, but only one of the dependencies are used at a time. Is this considered bad design?

public class OrderPayment
{
    ICreditCardPayment _ccPayment;
    ICashPayment _cashPayment;

    public OrderPayment(ICreditCardPayment ccPayment, ICashPayment cashPayment)
    {
        _ccPayment = ccPayment;
        _cashPayment = cashPayment;
    }

    private void PrepareOrder(Order order)
    {
        // Do stuff with the order
    }

    public PaymentResult PayByCreditCard(Order order)
    {
        PrepareOrder(order);
        return _ccPayment.Pay(order);
    }

    public PaymentResult PayByCreditCard(Order order)
    {
        PrepareOrder(order);
        return _cashPayment.Pay(order);
    }   
}

An alternative is this:

public class OrderPayment
{
    private void PrepareOrder(Order order)
    {
        // Do stuff with the order
    }

    public PaymentResult PayByCreditCard(Order order, ICreditCardPayment ccPayment)
    {
        PrepareOrder(order);
        return ccPayment.Pay(order);
    }

    public PaymentResult PayByCreditCard(Order order, ICashPayment cashPayment)
    {
        PrepareOrder(order);
        return cashPayment.Pay(order);
    }   
}

This one complicates the function call somewhat. Would you use the first, cleaner looking one, even though not every constructor parameter is used? Considering a DI framework has to instantiate potentially heavy classes even though they may not all be used, I'm not sure how good this is.

So which one would you use? Or maybe a different implementation?

I asked a very similar question here: http://stackoverflow.com/questions/6960686/is-the-di-pattern-limiting-wrt-expensive-object-creation-coupled-with-infrequent — ckittel, Aug 24 '11 at 14:22

score 1 · Accepted Answer · answered Aug 24 '11 at 14:41

I would refactor or extract a common interface from ICashPayment and ICreditCardPayment. Your code sample indicates your methods are both invoking xPayment.Pay, which looks like a good candidate for your common interface method.

public interface IPayment 
{
    PaymentResult Pay(Order order);
}

Your more specialized interfaces can inherit from and build upon it.

In general, I would avoid having constructors (or any method) accept arguments that go unused, or if one argument is used, the other is not. That's normally an indication that you are either not operating at the proper level of abstraction, or that your class/method has too many responsibilities.

score 0 · Answer 2 · answered Aug 24 '11 at 15:14

You need an operation that requires all of the following:

the payment method
the amount being paid
the order that is being paid for
What processing needs to be done prior to payment, based on the order, and amount being paid

You are trying to define some order of operation independent of what is being performed in the operation. Dependency injection can be used at the method level too.

you need a method like this one:

public PaymentResult Pay(Amount amount, Order order, IOrderService orderService,
    IPaymentService paymentService) {
  var updatedOrder = orderService.Process(order); // don't alter the original in
                                                  // case you need to roll back
  var result = paymentService.Pay(amount, updatedOrder);
  return result; // this result should include the updated order, so that the system
                 // can determine what to do upon successful payment
}

Ok not to use all parameters in constructor? - Revisited

2 Answers2