Applying DRY to an inheritance hierarchy

Question

I'm working on refactoring a legacy application where I implemented the State pattern successfully as shown in the following diagram:

As you see there is a common behavior between the 3 states, so I decided to extract the common method Refund() to a parent abstract class RefundableState where I implemented it:

Refactoring further I notice that the Ship() method is common between the 2 states Cancelled and NewOrder, so I go for another round of extracting the common behavior to another parent class ShippableState:

Now I need my Cancelled state to extend 2 classes and in the same time I can't group the Refund() and Ship() function together as not all state permit the 2 actions.

How can I fix this?

Notes:

• This question is mainly about applying DRY and refactoring not about how to implement the state pattern.
• I'm using PHP 5.3, but I'm open to answers in any other languages.

Answer 1

Why not keeping things simple? You can easily keep your code DRY without any extra classes (your first design), and opposed to the other answers here, IMHO there is no need to sacrifice the DRY principle for this case.

If the "Refund" method is equal in all three cases, or the "Ship" method in just two, implement those methods in "OrderState". If the implementation is only similar, but not identical, refactor the differences to virtual helper methods which are overriden in the subclasses (this is called template method pattern). And if you want to separate the interface from the implementation (which has nothing to do with "DRY", only with "separation of concerns"), then replace the class OrderState by an interface IOrderState, and let OrderState inherit from IOrderState, providing default implementations as well as reusable code for the other subclasses.

If the "Shipped" class does not need a "Ship" implementation at all, put the common functionality into a new method OrderState.DoShip, and call that method from Cancelled.Ship and NewOrder.Ship, but not from Shipped.Ship (the last one should throw an exception or do nothing, whatever is appropriate in your case).

And if the implementation of "Refund" or "Ship" methods reaches a certain size, and needs several helper methods and perhaps some state variables, then it is time to think about extracting those methods to helper classes like "Refunder" or "Shipper" (which are not getting part of your inheritance hierarchy).

Answer 2

In Java (don't know PHP), you could Create ShippableState, RefundableState (and CancellableState) as interfaces, create ShippableRefundableState class that implements ShippableState and RefundableState. But what about CancellableState? Without multiple implementation inheritance itcannot be fitted there somewhere without repeating it.

So maybe don't use inheritance, use composition. So have StateShipper/Refunder/Canceller with XXXState(state) method and delegate to it in your states.

In language with multiple implementation inheritance (eg. C++) you could create classes instead of interfaces and inherit from them in states. As long as multiple implementations does not multiply implement same thing, it is nice, awfull things starts when its not clear (to human) from which ancestor something is.

Answer 3

I can say only that "DRY" is not a silver bullet. You have to use the common sense when deciding if specific refactoring is needed.

In your example you can clearly see that there are problems when applying 100% DRY approach. I think that this particular refactoring in this scenario is not needed. Especially 2nd step.

But if you really want to do this then ...

I would advise you to drop Inheritance based design and start using Interface based design. This inheritance limit was created because developers where making "crazy" and unmanageable solutions. If you encounter multi inheritance problem you are clearly doing something wrong.

With interfaces you can use composition and dependency injection to store similar code in one place.

Answer 4

Although DRY is a good principle, you can also overdo it.

Even if each of the states supports a refund method, that does not mean you are repeating yourself. That would only happen if the business rules state that the refund handling must be identical in two or more states.
But even if there is common behaviour among a group of states, I don't see an advantage in adding multiple intermediate classes. Ultimately, all state classes in the pattern have to inherit from the same parent, so the common behaviour for several operations could also be bundled into that base class (especially if that common behaviour is to indicate "operation not supported").