7

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

State Pattern UML

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 for common operations

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:

Refactoring problem!!

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.
Improve this question

4 Answers 4

Reset to default
2

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).

Improve this answer
3

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.

Improve this answer
1
  • 2
    When you use inheritance so as to you new behaviour, prefer aggregation. The rule remains true even for clean C++ coding. Commented Feb 12, 2013 at 11:54
2

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.

Improve this answer
2

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").

Improve this answer
1
  • 1
    +1: I read this sentence this morning: A new level of indirection can solve all problems, except the problem of having too many levels of indirection. Love this sentence :-) Commented Feb 12, 2013 at 11:55

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.